New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8406] ProcessBuilderImpl catches exceptions thrown on background thread #7093

Merged
merged 1 commit into from Oct 3, 2018

Conversation

Projects
None yet
5 participants
@joshlemer
Copy link
Member

joshlemer commented Aug 19, 2018

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Aug 19, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 21, 2018

Since the Java is well-documented, I think you might as well add the other failure cases:

    override def run(io: ProcessIO): Process = {
      val process = try {
        p.start() // start the external process
      } catch {
        case _: IndexOutOfBoundsException
           | _: IOException
           | _: NullPointerException
           | _: SecurityException
           | _: UnsupportedOperationException
          => return FailedProcess
      }
      import io._

and tests, which I think could just go in the existing ProcessTest.scala which tests the stack:

  @Test def t8406(): Unit = {
    import java.io.ByteArrayInputStream
    assert(Try(("does-not-exist" #< new ByteArrayInputStream("dummy input".getBytes("utf-8"))).lazyLines).isSuccess)
    assert(Try(("does-not-exist" #< new ByteArrayInputStream("dummy input".getBytes("utf-8"))).lazyLines.toList).isFailure)
    assert(Try((Seq.empty[String] #< new ByteArrayInputStream("dummy input".getBytes("utf-8"))).lazyLines).isSuccess)
    assert(Try((Seq.empty[String] #< new ByteArrayInputStream("dummy input".getBytes("utf-8"))).lazyLines.toList).isFailure)
    assert(Try((Seq(null: String) #< new ByteArrayInputStream("dummy input".getBytes("utf-8"))).lazyLines).isSuccess)
    assert(Try((Seq(null: String) #< new ByteArrayInputStream("dummy input".getBytes("utf-8"))).lazyLines.toList).isFailure)
  }

I didn't add tests for all the failures. I changed the string to be descriptive. (And after many years, I've suddenly become sensitive to the origin of "foo".)

So this change is for the redirect case, which hangs because of the threading infrastructure. This is a Band-Aid, perhaps, but a process module could improve the architecture. For now, I wondered why .! is OK but .#< is not.

Actually, playing around in the 2.13 REPL, it hung in a way that I got the REPL prompt back, but then ctl-D didn't exit, presumably threads were still active.

Edit: sorry, hadn't mkPack yet. But it still hangs. The REPL transcript:

scala> import sys.process._, java.io._, scala.util._
import sys.process._
import java.io._
import scala.util._

scala> "does-not-exist" #< new ByteArrayInputStream("dummy input".getBytes("utf-8"))
res1: scala.sys.process.ProcessBuilder =  ( <input stream> #| [does-not-exist] )

scala> .lazyLines
res2: LazyList[String] = LazyList(?)

scala> .toList
java.lang.RuntimeException: Nonzero exit code: 1
  at scala.sys.package$.error(package.scala:26)
  at scala.sys.process.BasicIO$LazilyListed$.$anonfun$apply$1(BasicIO.scala:49)
  at scala.collection.immutable.LazyList$.$anonfun$unfold$1(LazyList.scala:710)
  at scala.collection.immutable.LazyList.state$lzycompute(LazyList.scala:196)
  at scala.collection.immutable.LazyList.state(LazyList.scala:194)
  at scala.collection.immutable.LazyList.isEmpty(LazyList.scala:203)
  at scala.collection.IterableOnceOps.nonEmpty(IterableOnce.scala:695)
  at scala.collection.IterableOnceOps.nonEmpty$(IterableOnce.scala:695)
  at scala.collection.AbstractIterable.nonEmpty(Iterable.scala:898)
  at scala.collection.immutable.LazyList$LazyIterator.hasNext(LazyList.scala:574)
[snip]
  at scala.collection.IterableOnceOps.toList$(IterableOnce.scala:1198)
  at scala.collection.AbstractIterable.toList(Iterable.scala:898)
  ... 33 elided

scala> 

scala> :quit [hangs here and doesn't exit]

So this looks like an improvement, but I'd like to know why there's an active thread. Here is the thread:

"Thread-4" #15 prio=5 os_prio=31 tid=0x00007fdb4dfa0000 nid=0xf07 waiting on condition [0x000070000b064000]
   java.lang.Thread.State: WAITING (parking)
	at sun.misc.Unsafe.park(Native Method)
	- parking to wait for  <0x00000007be5cf1d0> (a java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
	at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
	at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2039)
	at java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:442)
	at scala.sys.process.ProcessImpl$PipeSink.run(ProcessImpl.scala:204)
@som-snytt
Copy link
Contributor

som-snytt left a comment

Might as well cover the other documented exceptions.

@joshlemer

This comment has been minimized.

Copy link
Member Author

joshlemer commented Aug 21, 2018

Thank you for the feedback @som-snytt 🥇

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 21, 2018

I just updated the previous comment. This process stuff is complicated and brittle, so I wouldn't hold up this improvement because of the extra thread problem. (Assuming my test is legit.)

@joshlemer

This comment has been minimized.

Copy link
Member Author

joshlemer commented Aug 21, 2018

Yeah I realized how complicated this ProcessBuilder stuff is when trying to get to the root of scala/bug#10823

@joshlemer

This comment has been minimized.

Copy link
Member Author

joshlemer commented Aug 22, 2018

@som-snytt your requests are now implemented

@joshlemer joshlemer force-pushed the joshlemer:issue/8406-2.13 branch from c98efc9 to 0b832ba Sep 10, 2018

[8406] ProcessBuilderImpl catches exceptions thrown on background thread
[8406] Adds more caught exceptions in Simple ProcessBuilder#run

@joshlemer joshlemer force-pushed the joshlemer:issue/8406-2.13 branch from 0b832ba to 8b30c14 Sep 20, 2018

@adriaanm adriaanm merged commit f377f01 into scala:2.13.x Oct 3, 2018

3 checks passed

cla @joshlemer signed the Scala CLA. Thanks!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [4558] SUCCESS. Took 40 min.
Details

@joshlemer joshlemer deleted the joshlemer:issue/8406-2.13 branch Oct 3, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Oct 8, 2018

@joshlemer could this PR be the cause of the Windows failures that begin here? https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-windows/732/

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Oct 8, 2018

It looks like that test was due to me. It tries to run itself with $java.home/bin/java and consume the output of the subprocess. It falls back to running /bin/ls, so maybe it's not finding java, and previously trying to run ls would silently fail.

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Oct 16, 2018

@som-snytt okay, do you intend to pursue it further, or should I take it...?

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Oct 16, 2018

@SethTisue I kind of remember the thread but not quite, plus I have to bicycle over to the auto shop to get my car, a few miles, and hopefully drive to office without getting stranded, so you can take it. I know you can take it!

@SethTisue SethTisue referenced this pull request Oct 16, 2018

Closed

t6488 failing on Windows #566

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Oct 16, 2018

I'll pursue it further at scala/scala-dev#566

@joshlemer

This comment has been minimized.

Copy link
Member Author

joshlemer commented Oct 17, 2018

I'm confused, did this PR cause the issue?

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Oct 17, 2018

Looks like before, the failure to start the child would still let the input function run, but now it's not. Now I have to look at how the test is supposed to detect the failure.

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