SI-10007 sys.process thread sync #5481
Conversation
|
/rebuild |
4284367 to
15593ff
Compare
|
Rebased and changed test file name to scientific notation. :) |
|
/rebuild |
1 similar comment
|
/rebuild |
| class ProcessTest { | ||
| @Test def t10007(): Unit = { | ||
| val res = ("cat" #< new ByteArrayInputStream("lol".getBytes)).!! | ||
| assertEquals("lol\n", res) |
There was a problem hiding this comment.
Test won't fly on windows, sorry, @SethTisue
|
LGTM - except for that test on windows.. should we just guard it with |
15593ff to
1b02796
Compare
|
I added the SI-10055 test because nothing good happens after midnight. It doesn't really add anything but time. Or I guess it tests the lambda initialization. The call-by-name arg could be invoked anywhere, including a static init where it could block. Sorry, Jenkins, I was too sleepy to compile and test before bed, would you do that, there's a good chap. |
| @@ -91,12 +91,11 @@ private[process] trait ProcessImpl { | |||
|
|
|||
| protected lazy val (processThread, getExitValue, destroyer) = { | |||
| val code = new SyncVar[Option[Int]]() | |||
| code.put(None) | |||
| val thread = Spawn(code.put(runAndExitValue())) | |||
There was a problem hiding this comment.
Should this be try-finally like at ProcessBuilderImpl L61?
1b02796 to
f92a043
Compare
|
LGTM, thanks for these cleanups! |
|
Not sure if I just saw this now: could you rename |
A previous change to replace `SyncVar.set` with `SyncVar.put` breaks things. This commit tweaks the thread synchronizing in `sys.process` to actually use `SyncVar` to sync and pass a var. Joining the thread about to exit is superfluous. A result is put exactly once, and consumers use non-destructive `get`. Note that as usual, avoid kicking off threads in a static context, since class loading cycles are somewhat dicier with 2.12 lambdas. In particular, REPL is a static context by default. SI-10007 Clarify deprecation message The message on `set` was self-fulfilling, as it didn't hint that `put` has different semantics. So explain why `put` helps avoid errors instead of creating them. SI-10007 Always set exit value Always put a value to exit code, defaulting to None. Also clean up around tuple change to unfortunately named Future.apply. Very hard to follow those types. Date command pollutes output, so tweak test.
ed2d7b9 to
fbcfba2
Compare
|
Hopefully spurious. I renamed both those junit files and squashed. |
|
Test rig is failing badly. I won't rebuild at this time. |
|
/rebuild The OOMpas are at it again. Rig is feeling fresher now, it looks like. |
|
Thanks, @som-snytt |
The changes were made in scala#5481, subsequently breaking binary compatibility checks after scala#5532 was merged, too. The affected methods are part of an internal implementation class. Whitelisting should be safe.
A previous change to replace
SyncVar.setwithSyncVar.putbreaks things.
This commit tweaks the thread synchronizing in
sys.processto actually use
SyncVarto sync and pass a var.Joining the thread about to exit is superfluous.
A result is put exactly once, and consumers use
non-destructive
get.Note that as usual, avoid kicking off threads in a static
context, since class loading cycles are somewhat dicier
with 2.12 lambdas. In particular, REPL is a static context
by default.