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

Ensure a message is logged if we can not create an override #80

Merged
merged 2 commits into from Jun 9, 2016

Conversation

Projects
None yet
8 participants
@jtnord
Copy link
Member

commented Apr 11, 2016

If the first access of the ClassFilter is from a class being auto wired
then the root cause of the Issue can get swallowed and it is not clear in
the logs what is wrong. So if something goes wrong always log the error
at the expense that it gets logged twice (which is not a bad thing as it
is fatal anyway and will cause Jenkins to pretty much not work at all)

@reviewbybees

Ensure a message is logged if we can not create an override
If the first access of the ClassFilter is from a class being auto wired
then the root cause of the Issue can get swallowed and it is not clear in
the logs what is wrong.  So if something goes wrong always log the error
at the expense that it gets logged twice (which is not a bad thing as it
is fatal anyway and will cause Jenkins to pretty much not work at all)
@reviewbybees

This comment has been minimized.

Copy link

commented Apr 11, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@kzantow

This comment has been minimized.

Copy link

commented Apr 11, 2016

🐝

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@mslusarczyk

This comment has been minimized.

Copy link

commented Apr 12, 2016

🐝

@jtnord

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2016

@reviewbybees

This comment has been minimized.

Copy link

commented Apr 12, 2016

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.

}
catch (Error e) {
// when being used by something like XStream the actual cause gets swallowed
LOGGER.log(Level.SEVERE, "Failed to initialize the default class filter", e);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Apr 13, 2016

Member

I'm not a big fan of this construct, because it may hide the original OOM if a new one happens within Logger listeners. But good for other error types

This comment has been minimized.

Copy link
@jtnord

jtnord Apr 13, 2016

Author Member

this happens early on so if we are OOM there then wehey...
now you can argue this should be Throwable or a specific Error should be thrown.
or even a big fat 🐛 that List<Pattern> patternOverride = loadPatternOverride(); should be inside the try block.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

🐝 with a corner-case grumbling

@jtnord

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2016

🐛 on my own code... it's late and I need to take a look tomorrow, but despite all the positive reviews I think this is not correct.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 13, 2016

@jtnord any plans?
May be a subject for backporting

@jtnord

This comment has been minimized.

Copy link
Member Author

commented May 13, 2016

@oleg-nenashev need to fix it up, need to get some other bits done first so no a huge priority for me right now.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 13, 2016

Sure, thanks for update!

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 6, 2016

@jtnord Would be great to get it integrated by the end of this week. I'm planning a new release, which may still get into the LTS

@jtnord

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2016

removing my own bug with a 🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

🐝 from me

@amuniz

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

🐝

@jtnord

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2016

@reviewbybees done (again)

@jtnord jtnord added ready-to-merge and removed needs-review labels Jun 9, 2016

@oleg-nenashev oleg-nenashev merged commit f18c44e into jenkinsci:master Jun 9, 2016

1 check passed

Jenkins This pull request looks good
Details

oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Jun 10, 2016

Update remoting to 2.60
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)

oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Jun 11, 2016

Update remoting to 2.60 (#2403)
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)

olivergondza added a commit to jenkinsci/jenkins that referenced this pull request Jul 20, 2016

Update remoting to 2.60 (#2403)
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) -
Make the channel reader tolerant against Socket timeouts.
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) -
Support no_proxy environment variable.
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  -
Do not invoke PingFailureAnalyzer for agent=>master ping failures.
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) -
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection.
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) -
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class).
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>.
(jenkinsci/remoting#80)
(cherry picked from commit c718516)

samatdav added a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016

Update remoting to 2.60 (jenkinsci#2403)
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)

samatdav added a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016

Update remoting to 2.60 (jenkinsci#2403)
Changes summary:

Fixed issues:
* [JENKINS-22722](https://issues.jenkins-ci.org/browse/JENKINS-22722) - 
Make the channel reader tolerant against Socket timeouts. 
(jenkinsci/remoting#80)
* [JENKINS-32326](https://issues.jenkins-ci.org/browse/JENKINS-32326) - 
Support no_proxy environment variable. 
(jenkinsci/remoting#84)
* [JENKINS-35190](https://issues.jenkins-ci.org/browse/JENKINS-35190)  - 
Do not invoke PingFailureAnalyzer for agent=>master ping failures. 
(jenkinsci/remoting#85)
* [JENKINS-31256](https://issues.jenkins-ci.org/browse/JENKINS-31256) - 
 <code>hudson.Remoting.Engine#waitForServerToBack</code> now uses credentials for connection. 
(jenkinsci/remoting#87)
* [JENKINS-35494](https://issues.jenkins-ci.org/browse/JENKINS-35494) - 
Fix issues in file management in <code>hudson.remoting.Launcher</code> (main executable class). 
(jenkinsci/remoting#88)

Enhancements:
* Ensure a message is logged if remoting fails to override the default <code>ClassFilter</code>. 
(jenkinsci/remoting#80)
@jglick

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

FTR this looks like JDK-8051847.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.