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.

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

[FIXED JENKINS-39289] When a proxy fails, report what caused the channel to… #128

Merged
merged 1 commit into from Nov 4, 2016

Conversation

@kohsuke
Copy link
Member

commented Oct 27, 2016

… go down

Today, this requires out of bound knowledge about which connection the proxy was representing, then use other means to figure out why it has failed.

This exception chaining shortens that step and makes it easy to find the cause as to why the channel was shut down.

[JENKINS-39289] When a proxy fails, report what caused the channel to…
… go down

Today, this requires out of bound knowledge about which connection the
proxy was representing, then use other means to figure out why it has
failed.

This exception chaining shortens that step and makes it easy to find the
cause as to why the channel was shut down.
@oleg-nenashev
Copy link
Member

left a comment

Mostly LGTM, just needs to get a binary compatibility fix

* Clears the {@link #channel} to signify that the {@link Channel} has been closed and break any complex
* object cycles that might prevent the full garbage collection of the channel's associated object tree.
*/
public void clear() {
channel = null;
public void clear(Exception cause) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 27, 2016

Member

API breakage. Please keep the original method as a Deprecated(?) one with null cause

This comment has been minimized.

Copy link
@kohsuke

kohsuke Oct 27, 2016

Author Member

This is not a method on a public class, therefore no external code could have called it.

@@ -1725,11 +1738,27 @@ public Channel channel() {
}

/**
* If the channel is null, return the cause of the channel termination.
*/
public Exception cause() {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 27, 2016

Member

Nice2Have: CheckForNull

This comment has been minimized.

Copy link
@kohsuke

kohsuke Oct 27, 2016

Author Member

I did consider that initially, but when I actually wrote the client side of the code that uses it, I realized that it gets in the way --- one only calls cause() after verifying that channel()==null

@batmat
batmat approved these changes Oct 30, 2016
Copy link
Member

left a comment

🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 30, 2016

@kohsuke please do not release without this fix

On Oct 30, 2016 16:42, "Baptiste Mathus" notifications@github.com wrote:

@batmat approved this pull request.

🐝


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#128 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC3IoJ6KmDXLzG78_yhcrDAN3-AB3NYuks5q5J6pgaJpZM4Kh1GT
.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

Agreed with comments. Do we keep it in Jenkins 3 only? IMHO it can be backported if you restrict the new public API

@oleg-nenashev oleg-nenashev merged commit 0d8a2af into master Nov 4, 2016

1 check passed

Jenkins This pull request looks good
Details
oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this pull request Nov 9, 2016
Update remoting to 2.31 in the Jenkins core.
The change introduces one serious bugfix (JENKINS-39596) and a bunch of various diagnostics improvements.

Bugfixes:

* [JENKINS-39596](https://issues.jenkins-ci.org/browse/JENKINS-39596) -
Jenkins URL in `hudson.remoting.Engine` was always `null` since `3.0`.
It was causing connection failures of Jenkins JNLP agents when using Java Web Start.
([PR jenkinsci#131](jenkinsci/remoting#131))
* [JENKINS-39617](https://issues.jenkins-ci.org/browse/JENKINS-39617) -
`hudson.remoting.Engine` was failing to establish connection if one of the URLs parameter in parameters was malformed.
([PR jenkinsci#131](jenkinsci/remoting#131))

Improvements:

* [JENKINS-39150](https://issues.jenkins-ci.org/browse/JENKINS-39150) -
Add logic for dumping diagnostics across all the channels.
([PR jenkinsci#122](jenkinsci/remoting#122), [PR jenkinsci#125](jenkinsci/remoting#125))
* [JENKINS-39543](https://issues.jenkins-ci.org/browse/JENKINS-39543) -
Improve the caller/callee correlation diagnostics in thread dumps.
([PR jenkinsci#119](jenkinsci/remoting#119))
* [JENKINS-39290](https://issues.jenkins-ci.org/browse/JENKINS-39290) -
Add the `org.jenkinsci.remoting.nio.NioChannelHub.disabled` flag for disabling NIO (mostly for debugging purposes).
([PR jenkinsci#123](jenkinsci/remoting#123))
* [JENKINS-38692](https://issues.jenkins-ci.org/browse/JENKINS-38692) -
Add extra logging to help diagnosing `IOHub` Thread spikes.
([PR jenkinsci#116](jenkinsci/remoting#116))
* [JENKINS-39289](https://issues.jenkins-ci.org/browse/JENKINS-39289) -
 When a proxy fails, report what caused the channel to go down.
([PR jenkinsci#128](jenkinsci/remoting#128))
oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Nov 10, 2016
Update remoting to 2.31 in the Jenkins core. (#2628)
The change introduces one serious bugfix (JENKINS-39596) and a bunch of various diagnostics improvements.

Bugfixes:

* [JENKINS-39596](https://issues.jenkins-ci.org/browse/JENKINS-39596) -
Jenkins URL in `hudson.remoting.Engine` was always `null` since `3.0`.
It was causing connection failures of Jenkins JNLP agents when using Java Web Start.
([PR #131](jenkinsci/remoting#131))
* [JENKINS-39617](https://issues.jenkins-ci.org/browse/JENKINS-39617) -
`hudson.remoting.Engine` was failing to establish connection if one of the URLs parameter in parameters was malformed.
([PR #131](jenkinsci/remoting#131))

Improvements:

* [JENKINS-39150](https://issues.jenkins-ci.org/browse/JENKINS-39150) -
Add logic for dumping diagnostics across all the channels.
([PR #122](jenkinsci/remoting#122), [PR #125](jenkinsci/remoting#125))
* [JENKINS-39543](https://issues.jenkins-ci.org/browse/JENKINS-39543) -
Improve the caller/callee correlation diagnostics in thread dumps.
([PR #119](jenkinsci/remoting#119))
* [JENKINS-39290](https://issues.jenkins-ci.org/browse/JENKINS-39290) -
Add the `org.jenkinsci.remoting.nio.NioChannelHub.disabled` flag for disabling NIO (mostly for debugging purposes).
([PR #123](jenkinsci/remoting#123))
* [JENKINS-38692](https://issues.jenkins-ci.org/browse/JENKINS-38692) -
Add extra logging to help diagnosing `IOHub` Thread spikes.
([PR #116](jenkinsci/remoting#116))
* [JENKINS-39289](https://issues.jenkins-ci.org/browse/JENKINS-39289) -
 When a proxy fails, report what caused the channel to go down.
([PR #128](jenkinsci/remoting#128))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.