-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Drop jprocesses in favor of java.lang.ProcessHandle #16529
Conversation
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 0cb340c
|
0cb340c
to
7a6ffb6
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 7a6ffb6
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I think the old as well as the new solution have the same nasty trait of killing unrelated process instances when working locally (e.g. you have quarkus:dev running in some other project and run a Quarkus test in this repo).
But this is nothing to block this PR.
...ramework/devmode-test-utils/src/main/java/io/quarkus/test/devmode/util/DevModeTestUtils.java
Outdated
Show resolved
Hide resolved
7a6ffb6
to
2ca29d9
Compare
2ca29d9
to
8b5468a
Compare
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 2ca29d9
|
8b5468a
to
f79fd38
Compare
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 8b5468a
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building f79fd38
Full information is available in the Build summary check run. Test Failures⚙️ Gradle Tests - JDK 11 Windows #📦 integration-tests/gradle✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/devmode✖ 📦 integration-tests/ide-launcher✖ 📦 integration-tests/kotlin✖ ✖ 📦 integration-tests/scala✖ ✖ |
Looks like CI is unhappy. Thanks for testing the bot :). |
Gradle tests should be better with the memory tweaks I did in #16249. Maven test job seems to show that killing does not work properly?
|
f79fd38
to
60c6b91
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 60c6b91
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/devmode✖ 📦 integration-tests/ide-launcher✖ 📦 integration-tests/kotlin✖ ✖ 📦 integration-tests/scala✖ ✖ |
The test failures are very suspicious. These tests weren't flaky. |
Yes, looks very related, especially that port error log I commented on earlier. In the worst case, |
7f9340d
to
dd87e7c
Compare
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 7f9340d
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building dd87e7c
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/devmode✖ 📦 integration-tests/ide-launcher✖ 📦 integration-tests/kotlin✖ ✖ 📦 integration-tests/scala✖ ✖ |
FYI, I'm having a look at this in my fork. It's remarkable that only Windows jobs are affected. |
This doesn't look good: JDK-8176725 ProcessHandle arguments is always null
This also affects the Investigating workarounds... |
Ok, I think I found a good workaround. Will see how CI fares in my fork. If all goes to plan I'll create a fresh PR and close this one... |
@famod you can also push to my branch if you want:
|
dd87e7c
to
9fbdd7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, pushed to your branch @gastaldi
// Kill all processes that were (indirectly) spawned by the current process. | ||
// It's important to do it this way (instead of calling result.destroy() first) | ||
// because otherwise children of that process can become orphaned zombies. | ||
DevModeTestUtils.killDescendingProcesses(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This invocation order problem took the most time to figure out. I was able to see (on Windows, via Process Explorer) that sometimes a sub-branch of processes survives result.destroy()
, loosing that parent and thus also the transitive parent connection to the surefire test JVM process. That resulted in descendants()
not finding those "orphaned" processes anymore.
Fixes #16521