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.

Sign up for GitHub

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

Read from the process before waiting for exit #6031

Merged
merged 1 commit into from
Dec 9, 2019
Merged

Read from the process before waiting for exit #6031

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, thanks, this was coming from the first version of the patch.


try {
List<String> versionCommand = new ArrayList<>(nativeImage);
versionCommand.add("--version");

Process versionProcess = new ProcessBuilder(versionCommand.toArray(new String[0]))
.redirectErrorStream(true)
.start();
versionProcess.waitFor();
Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice, @gwenneg had issues with this when running inside a Docker container on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

Does adding waitFor() fix this issue? The stream should not return -1 until the process has exited, so they should have the same effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll try

Copy link
Contributor

Choose a reason for hiding this comment

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

waitFor does the trick. But I'm not sure if it is just the additional time spent to call the method?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created the PR #10562

try (BufferedReader reader = new BufferedReader(new InputStreamReader(versionProcess.getInputStream()))) {
graalVMVersion = reader.lines().filter((l) -> l.startsWith("GraalVM Version")).findFirst();
}
Expand Down