Exception handling in sys.process #7362
Conversation
|
@som-snytt I am flattered that you think I know anything about this |
|
@NthPortal A PR is the best place to share our lack of knowledge. |
|
the Jenkins run is green. let's see if anyone takes an interest in reviewing this. I don't know this code at all |
|
@SethTisue maybe spin off the module now? I'd volunteer to manage it a la scala-xml. |
|
wow, really? Adriaan and I had given up, at this point, on the idea of making more modules for 2.13, but that was because it looked like it wasn't going to happen unless someone from the core team did it. But with 2.13.0-RC1 now pushed out a bit, if you want to go ahead and do it, I'd be in favor. Obviously you'd have our help and support, both with the splitting (which I think would be a pretty straightforward copy-and-paste job), and with helping to drum up additional help from the community once the module has been made. @adriaanm wdyt? too late, or not? |
|
looking over those other tickets/threads, I don't see any explicit discussion of |
|
The other module could be called "better-properties". |
test/files/run/t6488.scala
Outdated
| @@ -39,9 +39,10 @@ object Test { | |||
| val reading = new CountDownLatch(1) | |||
| val count = new AtomicInteger | |||
| def counted = count.get | |||
| val command = s"${f.getAbsolutePath} -classpath ${javaClassPath} Test data" | |||
| val outdir = s"$userDir/test/files/run/t6488-run.obj" | |||
| val command = s"${f.getAbsolutePath} -classpath ${javaClassPath}${pathSeparator}${outdir} Test data" | |||
There was a problem hiding this comment.
Why do we need to manually add the outdir now, but it was not necessary before?
There was a problem hiding this comment.
No idea. I forgot my Indiana Jones archaeology hat. I could investigate when the windows build started failing.
There was a problem hiding this comment.
Ah I didn't get the sequence of events. The broken test on windows made you discover the underlying bug. After fixing that, you still fix the test.
Though on non-windows, the manual outdir is not necessary right? Maybe there's a bug in scala.util.Properties.javaClassPath? Maybe the test should run in a forked vm?
There was a problem hiding this comment.
Actually, I still don't know. I'd assumed it was the /bin/ls fallback exec that worked on linux, but no, it should have run java with a no such class output. I recall that windows will block if i/o streams aren't consumed, was that the platform difference?
There was a problem hiding this comment.
Guess what println(f) shows? C:\Program Files\Java\jdk1.8.0_152\jre\bin\java-rmi.exe :-) Anyway, that's not the issue.
The test out dir is missing from the classpath indeed, on linux and windows, so the test never reaches the data() part, but that doesn't really matter, as there just needs to be some output, and the process produces Error: Could not find or load main class Test.
I still don't see why it would hang on windows not on linux, maybe you do?
There was a problem hiding this comment.
Now that the Red Sox have clinched it, I'll find time to try it out on Windows. I bought a replacement Windows box a year ago to use for work, but I dread turning it on. I did wonder what exe the test was finding.
My previous comment about windows process blocking if out or err is not consumed is due to this happening to me in a unit test in 2003. That might have been blocking due to hitting a buffer limit, though.
It would be nice to have a words"" interpolator that uses CommandLineParser to split parts and takes args whole to render a Seq[String].
There was a problem hiding this comment.
retronym mentioned caveats at the commit to run tests in-process: 3b93517
(I missed the change at the old repo, so by now, I haven't yet thought about all the implications for tests.)
FSR, listing the obj directory fails when running in-process, after working around "partest.output" not set the same when in-process. However, forcing the test to fork makes it work.
It would be nicer if env was same for in/out-of-process. I'll try to get back to the mystery of why .obj is not listable, since compiler output is the same, it's not a virtual dir like in REPL, right?
|
re: the question of spinning off the module for 2.13, we discussed it internally and the conclusion was "sorry, feature freeze". there's a lot more to say than that, I have a bunch of notes on it, I'll write more soon, stand by. (@som-snytt, you might consider whether you're interested in working on making pulling libraries into the REPL easier, a la Ammonite? does that appeal? we could discuss further on |
|
@lrytz Thanks, reviewing is thankless. We discussed this at the office a propos this pr. I spent some time with this over the weekend, because I'd already done the crossword. This code has limited shelf-life, so no point spending time on it, but it was also my fault for approving the previous change to catch exceptions. |
|
@SethTisue no problem, it was just a thought so @lrytz wouldn't spend time reviewing it. He's too nice. I have a jline3 branch to get back to, then I'll try more ammonization of REPL. |
|
@som-snytt how confident are you in the fix? Sorry, hard question :-) |
|
@lrytz I really like the part where the threads get names you can read in a thread dump. This really just adjusts the previous fix for catching failure on start, by moving the catch up to compound level. Otherwise an i/o thread is started after the failure. The other fix is to the previous lazy lines business, to avoid hanging after failure. So both changes are confident improvements. |
|
@adriaanm @SethTisue for the purposes of semantics, modularization is not a feature, so a feature freeze would not apply. Did you notice David Freese in the world series? My joke was the cold should not bother him, because he is Mr Freese. |
test/files/run/t6488.scala
Outdated
| @@ -39,9 +39,10 @@ object Test { | |||
| val reading = new CountDownLatch(1) | |||
| val count = new AtomicInteger | |||
| def counted = count.get | |||
| val command = s"${f.getAbsolutePath} -classpath ${javaClassPath} Test data" | |||
| val outdir = s"$userDir/test/files/run/t6488-run.obj" | |||
| val command = s"${f.getAbsolutePath} -classpath ${javaClassPath}${pathSeparator}${outdir} Test data" | |||
There was a problem hiding this comment.
Guess what println(f) shows? C:\Program Files\Java\jdk1.8.0_152\jre\bin\java-rmi.exe :-) Anyway, that's not the issue.
The test out dir is missing from the classpath indeed, on linux and windows, so the test never reaches the data() part, but that doesn't really matter, as there just needs to be some output, and the process produces Error: Could not find or load main class Test.
I still don't see why it would hang on windows not on linux, maybe you do?
I tried that argument, it didn't work on Mr. Adriaan Freeze, aka the Snow Miser:
|
b1ad03b to
ff9f632
Compare
Given writing code is introducing bugs, removing code is removing bugs. So it would be a bug fix, which are always very welcome. |
|
fresh Jenkins run: |
|
Jenkins is green the only unresolved thing I can find above is Lukas's comment at |
PipeThreads also have names.
Improve handling of failure of a process to start. In a `CompoundProcess`, catch exceptions and set a special exit value. For `lazyLines`, also supply a value so that requesting thread doesn't hang. It would be nicer to stow the exception and rethrow instead of using special integers. Test for stale thread.
d7be37e to
2c6cd6a
Compare
Add contributed hello, world test, tweaked, with fix, which partially reverts a previous change that broke coordination between sequential process and its pipe.
9daf65d to
30eb428
Compare
|
one more Jenkins run : |
|
thank you, Som, for your careful attention to this. |
Fixes scala/scala-dev#566