typesafehub / akka-contrib-extra

ConductR Akka contributions
Other
9 stars 16 forks source link

Making the BlockingProcess more simple and reliable. #37

Closed viktorklang closed 9 years ago

viktorklang commented 9 years ago

See #34,

@huntc Does this work on Linux, as required?

huntc commented 9 years ago

Thanks for the simplifications. Please also note the build failures. Tests are not passing now for the blocking process.

viktorklang commented 9 years ago

@huntc

The BlockingProcessSpec looks fishy, would you mind explaining:

   "allow a blocking process that is blocked to be destroyed" in {
      val command = getClass.getResource("/sleep.sh").getFile
      new File(command).setExecutable(true)

      val probe = TestProbe()
     // following line <---- The Receiver forwards the Exited message to probe.ref
      val receiver = system.actorOf(Props(new Receiver(probe.ref, List.empty)), "receiver2")
      val process = system.actorOf(BlockingProcess.props(receiver, List(command)), "process2")

      probe.watch(process) // Probe receives Terminated signal when process exits (no matter why)

      probe.expectMsg(Receiver.Out("Starting"))

      system.stop(process)

      probe.expectTerminated(process, 10.seconds) <------ Race condition here between DeathWatch (Terminated) and the Exited message from the Receiver
    }

Is this race condition by design, and why?

viktorklang commented 9 years ago

@huntc So given my analysis, the reason that the test now fails is because the Exited signal is sent before BlockingProcess terminates, but with the "old" implementation it was the other way around.

So, assuming that the test case race condition is unintentional, I propose to fix the test to become deterministic.

viktorklang commented 9 years ago

@huntc Build seems to fail due to artifact resolution issues. As I haven't touched the deps I can only assume it's repo-related.

huntc commented 9 years ago

@viktorklang Are you ready for this to be merged?

viktorklang commented 9 years ago

@huntc yep!

viktorklang commented 9 years ago

@huntc Merge at your earliest convenience

huntc commented 9 years ago

Observed that this works fine with Ubuntu. I checked out this branch, published it locally and then used the local version from ConductR. I then ran ConductR via Docker (thus on Ubuntu) and was able to load/run a bundle, then send the SIGHUP to ConductR. All was well - the ConductR process was stopped.

huntc commented 9 years ago

Thanks very much @viktorklang for the nice simplifications.

viktorklang commented 9 years ago

Thanks @huntc! Glad I could help