Skip to content
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

Make -w and enter-to-rerun work with subprocess spawning tasks #1645

Merged
merged 3 commits into from
Dec 29, 2021

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Dec 29, 2021

The basic issue here is that we were not properly closing the input streams generated by spawnSubprocess when -i is not passed, in particular the one that pumps input from the proxied stdin to the subprocess. Thus it would survive long past the lifetime of the subprocess, try to pump the proxied stdin into the subprocess's closed stdin, and fail while consuming bytes from the proxied stdin so that other consumers couldn't get at it.

The fix is twofold:

  1. Pass in checkAvailable = true for the proxied-stdin-to-subprocess-inputstream pumper so it doesn't block on trying to read from stdin, and instead polls regularly at 2ms intervals
  2. Pass a java.util.function.BooleanSupplier runningCheck into the pumper that can be used to abort its polling at any point, and configure it so it aborts when its destination subprocess is no longer alive.

There's a small race condition where the subprocess may be alive when the check is made, but dead when the stream data is pumped a moment later, and so I add a try-catch around the InputPumper.run's dest.write call to just quietly discard the input and terminate the pumper

An unrelated change, I also added nice names to all the various threads we're spawning, so it's easier to keep track of them in the jstack.

Tested manually via ./mill -i dev.run scratch foo.run. On main without this PR, -i -w works:

lihaoyi mill$ ./mill -i dev.run scratch -i -w foo.run
[90/647] de.tobiasroeser.mill.vcs.version.VcsVersion.vcsState
[647/647] dev.run
[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)

But -w alone fails:

lihaoyi mill$ ./mill -i dev.run scratch  -w foo.run
[90/647] de.tobiasroeser.mill.vcs.version.VcsVersion.vcsState
[647/647] dev.run
[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

.run(InputPumper.java:29)
	... 1 more
<Enter>

Exception in thread "Thread-0" java.lang.RuntimeException: java.io.IOException: Read end dead
	at mill.main.client.InputPumper.run(InputPumper.java:34)
	at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.io.IOException: Read end dead
	at java.base/java.io.PipedInputStream.checkStateForReceive(PipedInputStream.java:262)
	at java.base/java.io.PipedInputStream.receive(PipedInputStream.java:226)
	at java.base/java.io.PipedOutputStream.write(PipedOutputStream.java:149)
	at mill.main.client.InputPumper.run(InputPumper.java:28)
	... 1 more

With this PR, both versions succeed:

lihaoyi mill$ ./mill -i dev.run scratch -i -w foo.run
[90/647] de.tobiasroeser.mill.vcs.version.VcsVersion.vcsState
[647/647] dev.run
[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
lihaoyi mill$ ./mill -i dev.run scratch -w foo.run
[90/647] de.tobiasroeser.mill.vcs.version.VcsVersion.vcsState
[647/647] dev.run
[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)
<Enter>

[44/44] foo.run
false
Watching for changes to 4 paths... (Enter to re-run, Ctrl-C to exit)

@lihaoyi lihaoyi closed this Dec 29, 2021
@lihaoyi lihaoyi reopened this Dec 29, 2021
@lihaoyi lihaoyi changed the title [WIP] Try and properly close InputPumpers to make -w work with subprocess spawning tasks Make -w and enter-to-rerun work with subprocess spawning tasks Dec 29, 2021
@lihaoyi lihaoyi requested a review from lefou December 29, 2021 04:39
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me. The thread naming is a bit inconsistent though. Pumper threads sometimes end with "Pump", "Pumper" or without any suffix.

@lihaoyi lihaoyi merged commit 5ce1aa7 into main Dec 29, 2021
@lefou lefou deleted the closepumpers branch December 29, 2021 12:33
@lefou lefou added this to the after 0.10.0-M5 milestone Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants