Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

SI-6488: Fix for race with open I/O fds #1522

Merged
merged 1 commit into from

3 participants

@possiblywrong

Unless the I/O threads for error and output are closed prior to Process.destroy() there is a potential race condition that ends in the I/O threads raising an IOException.

Added test case and resubmitted against 2.10.x

CLA also signed and confirmed.

Supersedes #1484 which may now be closed.

@scala-jenkins

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1482/

@scala-jenkins

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1482/

@possiblywrong

...as usual works for me fails in CI. Pointers on a reliable way to get the current scala runtime and the path to the compiled Test object would be welcome from a test would be much appreciated.

@jsuereth
Owner

You can't make any assumptions about what is available on the path in this test, sorry. We run on both windows + linux, and the current test will break many environments.

I think the best you can do is try the test, but not fail if the command is not found.

@possiblywrong

Can't I assume we have a scala runtime and the compiled test itself? This test really only requires those and the correct path to those.
My assumption was that with those I could ensure that this test did work in a portable way, not relying on anything in the environment.

@possiblywrong

Test case revised as suggested by @jsuereth. How do I provoke another CI run?

@scala-jenkins

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1487/

@scala-jenkins

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1487/

@jsuereth jsuereth commented on the diff
test/files/run/t6488.scala
@@ -0,0 +1,11 @@
+import sys.process._
+object Test {
+ // Program that prints "Success" if the command was successfully run then destroyed
+ // It will silently pass if the command "/bin/ls" does not exist
+ // It will fail due to the uncatchable exception in t6488 race condition
@jsuereth Owner

Awesome comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jsuereth
Owner

So, this LGTM. Any chance I can get you to git rebase -i and mark all but the first commit as 'fixup' so we get one solid commit? The intermediate broken commits can mess up our tooling.

THANKS MUCH for the patch. good work!

@possiblywrong

Ready to go with the rebase. Anything else you need from me? or shall I start looking for more bugs? ;-)

@jsuereth
Owner

Yeah, lgtm

@jsuereth jsuereth merged commit 1f9124b into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 26, 2012
  1. SI-6488: Stop I/O threads prior to Process destruction

    Declan Conlon authored
This page is out of date. Refresh to see the latest.
View
5 src/library/scala/sys/process/ProcessImpl.scala
@@ -222,7 +222,10 @@ private[process] trait ProcessImpl {
p.exitValue()
}
override def destroy() = {
- try p.destroy()
+ try{
+ outputThreads foreach (_.stop())
+ p.destroy()
+ }
finally inputThread.interrupt()
}
}
View
1  test/files/run/t6488.check
@@ -0,0 +1 @@
+Success
View
11 test/files/run/t6488.scala
@@ -0,0 +1,11 @@
+import sys.process._
+object Test {
+ // Program that prints "Success" if the command was successfully run then destroyed
+ // It will silently pass if the command "/bin/ls" does not exist
+ // It will fail due to the uncatchable exception in t6488 race condition
@jsuereth Owner

Awesome comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ def main(args: Array[String]) {
+ try Process("/bin/ls").run(ProcessLogger { _ => () }).destroy
+ catch { case _ => () }
+ println("Success")
+ }
+}
Something went wrong with that request. Please try again.