-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Allow disabling process killing on interruption #3293
Conversation
Plugins may want to do custom action when the execution of a remote process is interrupted. Currently it is not possible, because the process started on a slave is killed on interruption. My actual use case: https://groups.google.com/forum/#!msg/jenkinsci-dev/s6IkynOzdqE/2eFNKN9lAAAJ My solution is to have a flag called killWhenInterrupted, set by default to true, which prevents process killing on the slave when unset.
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.
Launcher
should retain the default behavior when killWhenInterrupted
is not invoked. My suggestion would be to invert flags and to use dontKillWhenInterrupted
or so
* Indicates whether the process should be killed on interruption. | ||
* | ||
* @return {@code this} | ||
* @since 2.107 |
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.
Needs to be removed before merge
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.
Done
boolean quiet, @CheckForNull String workDir, @Nonnull TaskListener listener) { | ||
private final boolean killWhenInterrupted; | ||
|
||
RemoteLaunchCallable(@Nonnull List<String> cmd, @CheckForNull boolean[] masks, @CheckForNull String[] env, |
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.
LGTM, no external usages in detached components and plugins.
core/src/main/java/hudson/Proc.java
Outdated
/** | ||
* Indicates whether the process should be killed on interruption. | ||
*/ | ||
protected boolean killWhenInterrupted = true; |
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.
I would suggest using final private
fields and getters
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.
It can't be final
since the value is set in the implementations.
Also it can't be private
in order to be accessible from implementations. Maybe I should add a setter and make it private?
core/src/main/java/hudson/Proc.java
Outdated
@@ -217,7 +222,13 @@ public LocalProc(String[] cmd,String[] env,InputStream in,OutputStream out, File | |||
public LocalProc(String[] cmd,String[] env,InputStream in,OutputStream out,OutputStream err,File workDir) throws IOException { | |||
this( calcName(cmd), | |||
stderr(environment(new ProcessBuilder(cmd),env).directory(workDir), err==null || err== SELFPUMP_OUTPUT), | |||
in, out, err ); | |||
in, out, err, true ); |
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.
Would be preferable to just call the new constructor below to avoid the logic duplication
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.
Done
@@ -181,6 +181,8 @@ public Computer getComputer() { | |||
*/ | |||
protected boolean reverseStdin, reverseStdout, reverseStderr; | |||
|
|||
protected boolean killWhenInterrupted; |
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.
🐛 it causes a regression in external API users. The default value will be false
though the logic was always killing processes before the change
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.
You are right. Flag was inverted.
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.
killWhenInterrupted flag was inverted.
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.
The code looks good to me. There are more formatting cleanup than real code though.
@apetres BTW, have you already prototyped a patch for your plugin with this change? If yes, it would be nice to have a link
@@ -80,7 +80,7 @@ | |||
* | |||
* | |||
* @author Kohsuke Kawaguchi | |||
* @see FilePath#createLauncher(TaskListener) | |||
* @see FilePath#createLauncher(TaskListener) |
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.
It would be preferable to keep refactorings in a separate pull request. But this PR is a non-backportable feature, so I can live with that
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.
Sorry for that, my IDE is on steroids. I reverted the refactorings.
@@ -441,6 +443,10 @@ public ProcStarter writeStdin() { | |||
return this; | |||
} | |||
|
|||
public ProcStarter dontKillWhenInterrupted() { |
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.
Add some Javadoc with @since TODO
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.
Done
core/src/main/java/hudson/Proc.java
Outdated
@@ -64,6 +64,11 @@ | |||
public abstract class Proc { | |||
protected Proc() {} | |||
|
|||
/** | |||
* Indicates that the process should not be killed on interruption. | |||
*/ |
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.
@since TODO
. Protected fields are a part of API
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.
Done
@@ -181,6 +181,8 @@ public Computer getComputer() { | |||
*/ | |||
protected boolean reverseStdin, reverseStdout, reverseStderr; | |||
|
|||
protected boolean dontKillWhenInterrupted; |
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.
Some documentation would be useful + @since TODO
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.
Done
Happy to approve once Javadoc is added |
How does this compare to |
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.
@oleg-nenashev here is the link to the prototype plugin: Itiviti/testcomplete-plugin@566ce25
@daniel-beck AFAICT |
Thanks @apetres ! |
No proposed changelog entry 👎 |
I don't see the problem with that IMO this PR is not well justified was merged prematurely. |
…)" This reverts commit e7fbce3.
@daniel-beck Actually checking environment of a standalone process is a big problem for Windows. In Jenkins this resolution most probably does not work properly for 32-bit systems now: jenkinsci/winp#48 So I would not rely on this logic in Jenkins now. But I agree that I merged it too prematurely, sorry about that |
@oleg-nenashev No worries, easy to revert and seems straightforward to submit again for further review. |
I'd also really like to see a test for this feature in core, if reasonably possible. |
Revert "Allow disabling process killing on interruption (#3293)"
I merged the revert PR #3339 @apetres This proposed change is not rejected, it was just prematurely merged, and given our release frequency and commitment to maintain API compatibility, not reverting would equal permanent acceptance starting later today with the release of 2.111. I'm sorry about this mess. We're of course open to a resubmission of this PR against the current master, ideally with the concerns expressed above addressed. |
Ok, so I should:
@daniel-beck about your concerns above:
Yes, I can. Since only one instance of TestComplete/TestExecute process can be run at a time, I can even detect it using the arguments, something like here. So I don't have to rely on env vars.
It's not clear for me how to kill a vetoed process from the plugin's side, since it only has access to the Proc object. Also IMO killing a vetoed process contradicts the concept of vetoing. Maybe I could run native killing instructions in a separate, joined process, after I ran my custom code (but IMO that's not really nice). |
@apetres Tests would be great! In general, what we expect from a PR is noted in the PR template at https://github.com/jenkinsci/jenkins/blob/master/.github/PULL_REQUEST_TEMPLATE.md I think a reasonable start to continue this review would be the existing code + tests. Note that you'll probably have to re-do the code changes since it was merged and reverted (unless you're a Git wizard and know things I don't). Additionally, an explanation why |
@daniel-beck For me this is an undesired side effect because I do want to kill the process after I run some logic on the plugin's side. Keeping it running after the build would cause an issue with future builds running on the same executor. This is how the logic my plugin implementation looks like, for more details check the link to the prototype implementation.
Now that I take a second look maybe implementing |
Or just add a flag that a given process should be killable after being processed by your custom logic. Still, this approach does seem nicer, so I don't consider that a blocker. |
Created a new PR with the desired changes. |
Plugins may want to do custom action when the execution of a remote process is interrupted. Currently it is not possible, because
the process started on a slave is killed on interruption.
My actual use case: https://groups.google.com/forum/#!msg/jenkinsci-dev/s6IkynOzdqE/2eFNKN9lAAAJ
My solution is to have a flag called killWhenInterrupted, set by default to true, which prevents process killing on the slave when unset.