From 0ffb9533bb724c987b05277d7f26140edef50953 Mon Sep 17 00:00:00 2001 From: Nadav Samet Date: Wed, 9 Oct 2024 13:22:03 -0700 Subject: [PATCH] Fix race condition in PosixPluginFrontend --- .github/workflows/ci.yml | 2 +- .../frontend/PosixPluginFrontendSpec.scala | 52 +++++++++++++++++++ .../frontend/MacPluginFrontend.scala | 36 ------------- .../frontend/PluginFrontend.scala | 1 - .../frontend/PosixPluginFrontend.scala | 19 +++---- .../protocbridge/ProtocIntegrationSpec.scala | 2 +- .../frontend/MacPluginFrontendSpec.scala | 15 ------ .../frontend/PosixPluginFrontendSpec.scala | 13 ----- build.sbt | 11 ++++ 9 files changed, 75 insertions(+), 76 deletions(-) create mode 100644 bridge-stress-test/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala delete mode 100644 bridge/src/main/scala/protocbridge/frontend/MacPluginFrontend.scala delete mode 100644 bridge/src/test/scala/protocbridge/frontend/MacPluginFrontendSpec.scala delete mode 100644 bridge/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3683632..7b4b9a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,7 +25,7 @@ jobs: key: ${{ runner.os }}-sbt-${{ hashFiles('**/*.sbt') }} - name: Compile and test run: | - sbt test + sbt bridgeStressTest/compile test shell: bash - name: Format check if: ${{ runner.os == 'Linux' }} diff --git a/bridge-stress-test/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala b/bridge-stress-test/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala new file mode 100644 index 0000000..0f0d4c6 --- /dev/null +++ b/bridge-stress-test/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala @@ -0,0 +1,52 @@ +package protocbridge.frontend + +import org.apache.commons.io.IOUtils +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.must.Matchers +import protocbridge.{ExtraEnv, ProtocCodeGenerator} + +import java.io.ByteArrayOutputStream +import scala.sys.process.ProcessIO +import scala.util.Random + +class PosixPluginFrontendSpec extends AnyFlatSpec with Matchers { + if (!PluginFrontend.isWindows) { + it must "execute a program that forwards input and output to given stream" in { + val random = new Random() + val toSend = Array.fill(123)(random.nextInt(256).toByte) + val toReceive = Array.fill(456)(random.nextInt(256).toByte) + val env = new ExtraEnv(secondaryOutputDir = "tmp") + + val fakeGenerator = new ProtocCodeGenerator { + override def run(request: Array[Byte]): Array[Byte] = { + request mustBe (toSend ++ env.toByteArrayAsField) + toReceive + } + } + + // Repeat 10,000 times since named pipes on macOS are flaky. + val repeatCount = 10000 + for (i <- 1 to repeatCount) { + if (i % 100 == 1) println(s"Running iteration $i of $repeatCount at ${java.time.LocalDateTime.now}") + + val (path, state) = PosixPluginFrontend.prepare( + fakeGenerator, + env + ) + val actualOutput = new ByteArrayOutputStream() + val process = sys.process + .Process(path.toAbsolutePath.toString) + .run(new ProcessIO(writeInput => { + writeInput.write(toSend) + writeInput.close() + }, processOutput => { + IOUtils.copy(processOutput, actualOutput) + processOutput.close() + }, _.close())) + process.exitValue() + actualOutput.toByteArray mustBe toReceive + PosixPluginFrontend.cleanup(state) + } + } + } +} diff --git a/bridge/src/main/scala/protocbridge/frontend/MacPluginFrontend.scala b/bridge/src/main/scala/protocbridge/frontend/MacPluginFrontend.scala deleted file mode 100644 index 6f33cf8..0000000 --- a/bridge/src/main/scala/protocbridge/frontend/MacPluginFrontend.scala +++ /dev/null @@ -1,36 +0,0 @@ -package protocbridge.frontend - -import java.nio.file.attribute.PosixFilePermission -import java.nio.file.{Files, Path} -import java.{util => ju} - -/** PluginFrontend for macOS. - * - * Creates a server socket and uses `nc` to communicate with the socket. We use - * a server socket instead of named pipes because named pipes are unreliable on - * macOS: https://github.com/scalapb/protoc-bridge/issues/366. Since `nc` is - * widely available on macOS, this is the simplest and most reliable solution - * for macOS. - */ -object MacPluginFrontend extends SocketBasedPluginFrontend { - - protected def createShellScript(port: Int): Path = { - val shell = sys.env.getOrElse("PROTOCBRIDGE_SHELL", "/bin/sh") - // We use 127.0.0.1 instead of localhost for the (very unlikely) case that localhost is missing from /etc/hosts. - val scriptName = PluginFrontend.createTempFile( - "", - s"""|#!$shell - |set -e - |nc 127.0.0.1 $port - """.stripMargin - ) - val perms = new ju.HashSet[PosixFilePermission] - perms.add(PosixFilePermission.OWNER_EXECUTE) - perms.add(PosixFilePermission.OWNER_READ) - Files.setPosixFilePermissions( - scriptName, - perms - ) - scriptName - } -} diff --git a/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala b/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala index 3b83cfa..579fee1 100644 --- a/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala +++ b/bridge/src/main/scala/protocbridge/frontend/PluginFrontend.scala @@ -137,7 +137,6 @@ object PluginFrontend { def newInstance: PluginFrontend = { if (isWindows) WindowsPluginFrontend - else if (isMac) MacPluginFrontend else PosixPluginFrontend } } diff --git a/bridge/src/main/scala/protocbridge/frontend/PosixPluginFrontend.scala b/bridge/src/main/scala/protocbridge/frontend/PosixPluginFrontend.scala index 65935e0..bea2fd4 100644 --- a/bridge/src/main/scala/protocbridge/frontend/PosixPluginFrontend.scala +++ b/bridge/src/main/scala/protocbridge/frontend/PosixPluginFrontend.scala @@ -39,16 +39,13 @@ object PosixPluginFrontend extends PluginFrontend { Future { blocking { + // Each of the following two calls below block until the shell script opens the + // corresponding pipe. This ensures that we are not writing to any of the pipes val fsin = Files.newInputStream(inputPipe) + val fsout = Files.newOutputStream(outputPipe) + val response = PluginFrontend.runWithInputStream(plugin, fsin, env) fsin.close() - - // Note that the output pipe must be opened after the input pipe is consumed. - // Otherwise, there might be a deadlock that - // - The shell script is stuck writing to the input pipe (which has a full buffer), - // and doesn't open the write end of the output pipe. - // - This thread is stuck waiting for the write end of the output pipe to be opened. - val fsout = Files.newOutputStream(outputPipe) fsout.write(response) fsout.close() } @@ -77,8 +74,12 @@ object PosixPluginFrontend extends PluginFrontend { "", s"""|#!$shell |set -e - |cat /dev/stdin > "$inputPipe" - |cat "$outputPipe" + |exec 4> "$inputPipe" + |exec 5< "$outputPipe" + |cat /dev/stdin >&4 + |exec 4>&- + |cat <&5 + |exec 5<&- """.stripMargin ) val perms = new ju.HashSet[PosixFilePermission] diff --git a/bridge/src/test/scala/protocbridge/ProtocIntegrationSpec.scala b/bridge/src/test/scala/protocbridge/ProtocIntegrationSpec.scala index 606457a..f5ada53 100644 --- a/bridge/src/test/scala/protocbridge/ProtocIntegrationSpec.scala +++ b/bridge/src/test/scala/protocbridge/ProtocIntegrationSpec.scala @@ -156,7 +156,7 @@ class ProtocIntegrationSpec extends AnyFlatSpec with Matchers { Await.result( Future.sequence(invocations), - Duration(60, SECONDS) + Duration(120, SECONDS) ) must be(List.fill(parallelProtocInvocations)(0)) } } diff --git a/bridge/src/test/scala/protocbridge/frontend/MacPluginFrontendSpec.scala b/bridge/src/test/scala/protocbridge/frontend/MacPluginFrontendSpec.scala deleted file mode 100644 index 6e8b972..0000000 --- a/bridge/src/test/scala/protocbridge/frontend/MacPluginFrontendSpec.scala +++ /dev/null @@ -1,15 +0,0 @@ -package protocbridge.frontend - -class MacPluginFrontendSpec extends OsSpecificFrontendSpec { - if (PluginFrontend.isMac) { - it must "execute a program that forwards input and output to given stream" in { - val state = testSuccess(MacPluginFrontend) - state.serverSocket.isClosed mustBe true - } - - it must "not hang if there is an error in generator" in { - val state = testFailure(MacPluginFrontend) - state.serverSocket.isClosed mustBe true - } - } -} diff --git a/bridge/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala b/bridge/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala deleted file mode 100644 index 4a6dd99..0000000 --- a/bridge/src/test/scala/protocbridge/frontend/PosixPluginFrontendSpec.scala +++ /dev/null @@ -1,13 +0,0 @@ -package protocbridge.frontend - -class PosixPluginFrontendSpec extends OsSpecificFrontendSpec { - if (!PluginFrontend.isWindows && !PluginFrontend.isMac) { - it must "execute a program that forwards input and output to given stream" in { - testSuccess(PosixPluginFrontend) - } - - it must "not hang if there is an OOM in generator" in { - testFailure(PosixPluginFrontend) - } - } -} diff --git a/build.sbt b/build.sbt index 8643e66..4354f8c 100644 --- a/build.sbt +++ b/build.sbt @@ -46,6 +46,17 @@ lazy val bridge: Project = project ) ) +lazy val bridgeStressTest = project + .in(file("bridge-stress-test")) + .dependsOn(bridge) + .settings( + name := "bridge-stress-test", + libraryDependencies ++= Seq( + "org.scalatest" %% "scalatest" % "3.2.19" % "test", + "commons-io" % "commons-io" % "2.11.0" % "test" + ) + ) + lazy val protocCacheCoursier = project .in(file("protoc-cache-coursier")) .dependsOn(bridge)