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
[JENKINS-26947] forcibly terminate Maven remoting channel when upstream channel get closed #39
Conversation
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
Some test are failing on CI. Had the same failure on my laptop on the master branch |
@kohsuke would it maybe make sense to get this included in remoting ? Not sure channel-in-channel is a common pattern |
7dc8ada
to
6cf89d6
Compare
No, only I guess I am +0. If it fixes the problem then we need it. The patch is somewhat scary. |
} | ||
|
||
/** | ||
* copy/paste of Channels.forProcess |
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.
If there is a particular reason the existing method cannot be reused, it should be explained here.
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.
Channel.terminate() has protected visibility so LinkedChannelCleaner.onClose() can't call it directly
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.
Did you tried to move this hack-ish class to same hudson.remoting package so you can access it's protected method ?
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.
hudson.remoting
package is in a signed jar file, so you will be SOoL if you try that
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be |
🐝 |
@reviewbybees done |
This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging. |
@jglick Would you consider merging this PR? I can rebase to master and rerun test. I know it's pretty invasive but it's tested in real condition for some time now (there's a very big CloudBees DEV@cloud customer running fine with this patch) |
It has a bee but it needs a merge. |
@ydubreuil please rebase then we can merge |
Rebase or just |
…stream channel get closed Currently, when the main remoting channel is abruptly closed, Maven channel can be stuck for a while because it doesn't get notified of the disconnection
6cf89d6
to
47b2873
Compare
I've just rebased to master. |
[JENKINS-26947] forcibly terminate Maven remoting channel when upstream channel get closed
JENKINS-26947
Currently, when the main remoting channel is abruptly closed, Maven channel can be stuck for a while because it doesn't get notified of the disconnection.
@reviewbybees