Read from the process before waiting for exit #6031
Conversation
If you do not read from the Process InputStream is it possible that the process will block if the process buffer is too small to contain the full output. There is no need to call waitFor as reading the InputStream will have the same effect.
|
Windows failure appears to be due to #6032 |
| @@ -134,15 +134,15 @@ public NativeImageBuildItem build(NativeConfig nativeConfig, NativeImageSourceJa | |||
| nativeImage = Collections.singletonList(getNativeImageExecutable(graal, java, env).getAbsolutePath()); | |||
| } | |||
|
|
|||
| Optional<String> graalVMVersion = Optional.empty(); | |||
| final Optional<String> graalVMVersion; | |||
There was a problem hiding this comment.
Ah yes, thanks, this was coming from the first version of the patch.
| .start(); | ||
| versionProcess.waitFor(); |
There was a problem hiding this comment.
Ah, nice, @gwenneg had issues with this when running inside a Docker container on Windows.
There was a problem hiding this comment.
For me it is not clear why you have removed waiting to finish? I want to provide the container name flag using containerRuntimeOptions which fails because of the container already exist in the docker daemon.
The daemon is just not cleanup fast enough!
There was a problem hiding this comment.
Does adding waitFor() fix this issue? The stream should not return -1 until the process has exited, so they should have the same effect.
There was a problem hiding this comment.
waitFor does the trick. But I'm not sure if it is just the additional time spent to call the method?
There was a problem hiding this comment.
Maybe the stream gets closed as part of the process exit, but before the exit is actually complete, so it is still necessary to call waitFor(). Probably safest just to add it back, can you open a PR?
There was a problem hiding this comment.
I'm ready to create the PR. I have tried to find a unit/integration test to adjust (or copy) to reproduce the possible race condition. But I'm unsure which to use. Any help on choosing one?
There was a problem hiding this comment.
I don't think you will be able to test for it, as it is a race you can't actually prove that it is fixed with a test , and the test will be slow and require a lot of code too not really prove anything, I think it just needs to be a one liner.
If you do not read from the Process InputStream is it
possible that the process will block if the process buffer
is too small to contain the full output.
There is no need to call waitFor as reading the InputStream
will have the same effect.