Skip to content

Stop forked exec and javaexec on build termination#18756

Closed
vlsi wants to merge 4 commits into
gradle:masterfrom
vlsi:windows_taskkill
Closed

Stop forked exec and javaexec on build termination#18756
vlsi wants to merge 4 commits into
gradle:masterfrom
vlsi:windows_taskkill

Conversation

@vlsi

@vlsi vlsi commented Oct 24, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Terminating a single process does not terminate its children automatically, so when Gradle terminates .bat file, it leaves the forked application running.

The problem is often visible as "process exec process keeps running after build termination".
The root cause is that in Windows exec uses cmd -> process.exe sequence, so aborting the build terminates cmd.exe, and process.exe keeps running.

The workaround in Windows is to use taskkill /PID $pid /T /F command that terminates the process tree.

For other operating systems, alternative solutions are needed.
THe PR uses Process#descendants if Gradle runs with Java 9+
Otherwise, it resorts to the previous process.destroy() sequence.

Note: Java 8 does not have API for retrieving descendant processes, so something like https://unix.stackexchange.com/a/124148 is needed (see "recursive shell function" there) for Linux/macOS when Gradle runs with Java 8.

Fixes #1109 #1128 #3093 #6114 #7603 #18716:

the plot of fixed issues

Context

It would make .exec forked process more user-friendly.
The same approach works great for allure-gradle: allure-framework/allure-gradle#78

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass sanity check: ./gradlew sanityCheck
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

@vlsi vlsi force-pushed the windows_taskkill branch from a5faf5e to f80a392 Compare October 24, 2021 12:10
@stale

stale Bot commented Dec 24, 2021

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please ask the team to reopen the PR or push a new PR. Thank you for your contributions.

@stale stale Bot added the stale label Dec 24, 2021
@vlsi

vlsi commented Dec 24, 2021

Copy link
Copy Markdown
Contributor Author

I will update the PR soon, so please keep it open for a while.

@stale stale Bot removed the stale label Dec 24, 2021
@freynder freynder mentioned this pull request Feb 23, 2022
Closed
@stale

stale Bot commented Mar 27, 2022

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please ask the team to reopen the PR or push a new PR. Thank you for your contributions.

@stale stale Bot added the stale label Mar 27, 2022
@big-guy big-guy added the from:contributor PR by an external contributor label Apr 1, 2022
@vlsi vlsi force-pushed the windows_taskkill branch from 9b5abd0 to 354aae1 Compare April 7, 2022 19:14
@stale stale Bot removed the stale label Apr 7, 2022
@vlsi vlsi changed the title Stop forked .bat files on build termination Stop forked processes files on build termination Apr 8, 2022
@vlsi vlsi force-pushed the windows_taskkill branch 2 times, most recently from f8fa003 to d91a7a3 Compare April 8, 2022 20:46
@vlsi vlsi changed the title Stop forked processes files on build termination Stop forked exec and javaexec on build termination Apr 8, 2022
@vlsi vlsi force-pushed the windows_taskkill branch from d91a7a3 to dede11b Compare April 8, 2022 21:09
@vlsi

vlsi commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

I believe the PR is in a good shape, so it would be nice if somebody could review/merge it.

@vlsi
Stop forked exec and javaexec on build termination
f0a2207 
Terminating a single process does not terminate its children automatically,
so when Gradle terminates .bat file, it leaves the forked application running.

The problem is often visible as "process exec process keeps running after build termination".
The root cause is that in Windows exec uses cmd -> process.exe sequence,
so aborting the build terminates "cmd.exe", and "process.exe" keeps running.

The workaround is to use taskkill /PID $pid /T /F command that terminates the process tree

For other operating systems, alternative solutions are needed.
This commit uses Process#descendants if Gradle runs with Java 9+
Otherwise it resorts to the previous "process.destroy" sequence.

fixes gradle#18716 gradle#7603 gradle#6114 gradle#3093 gradle#1128 gradle#1109

Signed-off-by: Vladimir Sitnikov <sitnikov.vladimir@gmail.com>
@vlsi vlsi force-pushed the windows_taskkill branch from dede11b to f0a2207 Compare April 9, 2022 08:15
@vlsi vlsi mentioned this pull request Apr 9, 2022
Open
@eskatos eskatos self-assigned this Jun 7, 2022
@vlsi vlsi mentioned this pull request Jun 8, 2022
Closed
@gradle gradle deleted a comment from eskatos Jun 8, 2022
@gradle gradle deleted a comment from eskatos Jun 8, 2022
@gradle gradle deleted a comment from eskatos Jun 8, 2022
@blindpirate

blindpirate commented Jun 9, 2022

Copy link
Copy Markdown
Member

Sorry for the noise here, but the GitHub webhook events were not delivered successfully to our bot application because of CloudFlare. No actions required on your side, I'll take care of it.

@eskatos

eskatos commented Jun 10, 2022

Copy link
Copy Markdown
Member

Hi @vlsi,
Thanks for this PR.

There are some CI failures, see https://builds.gradle.org/buildConfiguration/Gradle_Master_Check_Quick_2_Trigger/52824264?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=false&expandBuildProblemsSection=true&expandPull+Request+Details=true

It seems the test executors are dying unexpectedly on Windows. But they should not as the Gradle version running the tests hasn't the changed code. Something must be wrong somehow. My Windows-fu is all rusty and I couldn't reproduce locally yet. Could you please take a look?

@vlsi

vlsi commented Jun 10, 2022

Copy link
Copy Markdown
Contributor Author

@eskatos , what are the credentials?

@ov7a ov7a removed the to-triage label Aug 26, 2024
@ov7a ov7a removed the to-triage label Sep 12, 2024
@ov7a ov7a removed the to-triage label Sep 30, 2024
@ov7a ov7a removed the to-triage label Oct 23, 2024
@ov7a ov7a removed the to-triage label Nov 7, 2024
@ov7a ov7a removed the to-triage label Nov 28, 2024
@ljacomet ljacomet mentioned this pull request Dec 5, 2024
Open
@ov7a ov7a removed the to-triage label Dec 25, 2024
@ov7a ov7a removed the to-triage label Jan 14, 2025
@ov7a ov7a removed the to-triage label Feb 11, 2025
@ljacomet ljacomet removed the to-triage label Mar 4, 2025
@ov7a ov7a removed the to-triage label Mar 20, 2025
@ov7a ov7a mentioned this pull request Apr 1, 2025
Open
@ov7a ov7a removed the to-triage label Apr 7, 2025
@erikvanderwerf

erikvanderwerf commented May 10, 2025

Copy link
Copy Markdown

It looks like this has been in stasis for the last 11 months. Does this just need to be tested / is there automation to test it?

@ov7a

ov7a commented May 12, 2025

Copy link
Copy Markdown
Member

It looks like this has been in stasis for the last 11 months. Does this just need to be tested / is there automation to test it?

Sorry, it's on our list, but we are buried under other priorities for now. We still don't have an ETA for it. As stated in the comment above, we need to validate this, which includes both design review and testing.

@jjohannes jjohannes mentioned this pull request May 23, 2025
Merged
8 tasks
@ljacomet

ljacomet commented Jul 16, 2025

Copy link
Copy Markdown
Member

Hey @vlsi,

Thank you for attempting to fix this and sorry it took so long for the Gradle team to wrap it up.

I am going to close this, since we have a more recent attempt that's being reviewed and is leveraging the fact that Gradle now can use 9+ APIs for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bric3 bric3 bric3 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ljacomet ljacomet

Labels

from:contributor PR by an external contributor in:exec-tasks

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Gradle doesn't stop forked processes Ctrl-C when running Spring Application get no shutdown hooks