Provide a non-blocking process implementation #60

huntc · 2017-08-12T06:10:44Z

Utilises NuProcess in order to provide a non-blocking process implementation for Windows, OS X and Linux.

huntc · 2017-08-14T07:43:39Z

@ktoso ... in case you're interested in reviewing further :-)

@longshorej ... in case you're interested integrating with our code base - we'll need to cherry pick changes to an Akka 2.4 flavour of this lib though (shouldn't be hard).

longshorej · 2017-08-14T18:29:18Z

Branch created @ https://github.com/typesafehub/akka-contrib-extra/tree/3.0, I'll backport this PR once merged.

longshorej · 2017-08-14T19:56:47Z

src/main/scala/akka/contrib/process/NonBlockingProcess.scala

+   * @param stderr a `akka.stream.scaladsl.Source[ByteString]` for the standard error stream of the process
+   */
+  case class Started(
+    pid: Long,


~~I think pid: Int -- NuProcess.getPID() returns an int~~

On second thought, ignore.

Yeah, I was influenced here by the forthcoming JDK process API (which we'll probably never use!)

huntc · 2017-08-15T07:16:57Z

Hey, @viktorklang - would you care to review - I think this PR would be right up your alley. :-)

huntc · 2017-08-15T07:52:01Z

Tests are failing fairly consistently on Travis with:

[info] - should read from stdin and write to stdout *** FAILED ***
[info]   java.lang.AssertionError: assertion failed: timeout (6 seconds) during expectMsg:
...
[info]   at akka.contrib.process.NonBlockingProcessSpec$$anonfun$1$$anonfun$apply$mcV$sp$4.apply(NonBlockingProcessSpec.scala:39)

I don't seem to be able to reproduce this locally. I've enhanced the test build config to cut out parallelism etc. but still no cigar...

huntc · 2017-08-15T08:21:12Z

Update on #60 (comment)

I can reproduce this problem with reasonable consistency on Linux - harder to reproduce when invoking the "read from stdin and write to stdout" test, but not impossible. There may still be something racey about my custom graph...

huntc · 2017-08-16T03:12:56Z

@longshorej I've disabled that test you added a while back in the interest of pushing this forward. Would you mind re-visiting that test? It isn't making much sense to me. For starters, the test declares, "detect when a process has exited of its own accord", but it isn't - a Destroy is being sent...

huntc · 2017-08-17T07:39:00Z

@longshorej I’ve re-instated that test of yours for NonBlockingProcess and implemented the polling on /proc. Unfortunately, the test fails on Linux (passing just fine on OS X). I’ve run out of time today, but I was wondering if you could continue taking a look at it...

longshorej · 2017-08-17T16:34:31Z

src/main/scala/akka/contrib/process/NonBlockingProcess.scala

+ */
+class NonBlockingProcess(
+  command: immutable.Seq[String],
+  directory: File,


Should we take a Path instead and thus fully be using java.nio for everything?

huntc · 2017-08-18T00:19:45Z

src/main/scala/akka/contrib/process/NonBlockingProcess.scala

-        }
+      val isRunning = process.isRunning match {
+        case true if hasProcDir => procDirAlive(process.getPID)(contextMat, implicitly)
+        case result             => Future.successful(result)


We weren't handling non-Linux scenarios. :-)

huntc · 2017-08-18T00:20:28Z

src/test/scala/akka/contrib/process/BlockingProcessSpec.scala

@@ -20,36 +21,37 @@ class BlockingProcessSpec extends WordSpec with Matchers with BeforeAndAfterAll

  implicit val system = ActorSystem("test", testConfig)

-  implicit val processCreationTimeout = Timeout(2.seconds)
+  implicit val processCreationTimeout = Timeout(2.seconds.dilated)


In case we ever declare a test timeout factor

huntc · 2017-08-18T00:21:05Z

src/test/scala/akka/contrib/process/BlockingProcessSpec.scala

      val stdinInput = List("abcd", "1234", "quit")
-      val receiver = system.actorOf(Props(new Receiver(probe.ref, command, stdinInput, 1)), "receiver1")
+      val receiver = system.actorOf(Props(new Receiver(streamProbe.ref, exitProbe.ref, command, stdinInput, 1)), "receiver1")


Introduced the distinction between stream and exit probes as it is technically possible to get an exit prior to the stream data

huntc · 2017-08-18T00:21:24Z

src/test/scala/akka/contrib/process/BlockingProcessSpec.scala

@@ -20,36 +21,37 @@ class BlockingProcessSpec extends WordSpec with Matchers with BeforeAndAfterAll

  implicit val system = ActorSystem("test", testConfig)

-  implicit val processCreationTimeout = Timeout(2.seconds)
+  implicit val processCreationTimeout = Timeout(2.seconds.dilated)

  "A BlockingProcess" should {


Some small fixes to the blocking tests here.

huntc · 2017-08-18T00:21:55Z

src/test/scala/akka/contrib/process/NonBlockingProcessSpec.scala

-        case NonBlockingProcess.Exited(255) => true
-        case _                              => false
+        case NonBlockingProcess.Exited(_) => true
+        case _                            => false


In the case of non-Linux - we can get any non zero code.

huntc · 2017-08-18T00:25:48Z

src/test/scala/akka/contrib/process/NonBlockingProcessSpec.scala

+      process ! NonBlockingProcess.Destroy
+
+      exitProbe.fishForMessage() {
+        case NonBlockingProcess.Exited(s) if s != 0 => true


In the case of non-proc checking, we can get any non-zero

Utilises NuProcess in order to provide a non-blocking process implementation for Windows, OS X and Linux. We've also modified some of the blocking tests in order to improve their robustness.

longshorej

Nice!

huntc force-pushed the nuprocess branch from 7cb2548 to fd903aa Compare August 14, 2017 07:41

longshorej reviewed Aug 14, 2017

View reviewed changes

huntc force-pushed the nuprocess branch 3 times, most recently from d49f01d to 7d16a09 Compare August 15, 2017 07:15

huntc force-pushed the nuprocess branch 3 times, most recently from 0b6eb29 to 31d9460 Compare August 15, 2017 07:45

huntc force-pushed the nuprocess branch from c1708fe to 34cfd42 Compare August 15, 2017 23:28

huntc mentioned this pull request Aug 15, 2017

Reactive Streams Support brettwooldridge/NuProcess#71

Open

huntc force-pushed the nuprocess branch from 58d6267 to 86bba22 Compare August 16, 2017 03:55

longshorej reviewed Aug 17, 2017

View reviewed changes

huntc commented Aug 18, 2017

View reviewed changes

huntc force-pushed the nuprocess branch from 9b32499 to 34f60de Compare August 18, 2017 00:24

huntc commented Aug 18, 2017

View reviewed changes

Provide a non-blocking process implementation

ca2c982

Utilises NuProcess in order to provide a non-blocking process implementation for Windows, OS X and Linux. We've also modified some of the blocking tests in order to improve their robustness.

huntc force-pushed the nuprocess branch from 2800391 to ca2c982 Compare August 18, 2017 00:47

huntc changed the title ~~WIP - DO NOT MERGE - Provide a non-blocking process implementation~~ Provide a non-blocking process implementation Aug 18, 2017

longshorej approved these changes Aug 18, 2017

View reviewed changes

huntc merged commit f24ec4c into typesafehub:master Aug 18, 2017

huntc deleted the nuprocess branch August 18, 2017 01:22

huntc mentioned this pull request Aug 18, 2017

Provide a non-blocking process implementation #64

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a non-blocking process implementation #60

Provide a non-blocking process implementation #60

huntc commented Aug 12, 2017 •

edited

Loading

huntc commented Aug 14, 2017

longshorej commented Aug 14, 2017

longshorej Aug 14, 2017 •

edited

Loading

huntc Aug 14, 2017

huntc commented Aug 15, 2017

huntc commented Aug 15, 2017

huntc commented Aug 15, 2017

huntc commented Aug 16, 2017

huntc commented Aug 17, 2017

longshorej Aug 17, 2017

huntc Aug 18, 2017

huntc Aug 18, 2017

huntc Aug 18, 2017

huntc Aug 18, 2017

huntc Aug 18, 2017

huntc Aug 18, 2017

longshorej left a comment

Provide a non-blocking process implementation #60

Provide a non-blocking process implementation #60

Conversation

huntc commented Aug 12, 2017 • edited Loading

huntc commented Aug 14, 2017

longshorej commented Aug 14, 2017

longshorej Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntc commented Aug 15, 2017

huntc commented Aug 15, 2017

huntc commented Aug 15, 2017

huntc commented Aug 16, 2017

huntc commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longshorej left a comment

Choose a reason for hiding this comment

huntc commented Aug 12, 2017 •

edited

Loading

longshorej Aug 14, 2017 •

edited

Loading