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-28062] A Channel.Listener's onClose method that propagate... #42

Merged
merged 1 commit into from Apr 23, 2015

Conversation

Projects
None yet
4 participants
@stephenc
Copy link
Member

commented Apr 23, 2015

...s an exception is a sign of a bad listener not a problem closing the channel.

@reviewbybees

[FIXED JENKINS-28062] A Channel.Listener's onClose method that propag…
…ates an exception is a sign of a bad listener not a problem closing the channel.
try {
l.onClosed(this, e);
} catch (Throwable t) {
LogRecord lr = new LogRecord(Level.SEVERE, "Listener {0} propagated an exception for channel {1}'s close: {2}");

This comment has been minimized.

Copy link
@jtnord

jtnord Apr 23, 2015

Member

This would be so much cleaner with slf4j...

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 23, 2015

Author Member

Agree, but Jenkins uses JUL

This comment has been minimized.

Copy link
@jtnord

jtnord Apr 23, 2015

Member

yeah, a pet peve of mine.

} catch (Throwable t) {
LogRecord lr = new LogRecord(Level.SEVERE, "Listener {0} propagated an exception for channel {1}'s close: {2}");
lr.setThrown(t);
lr.setParameters(new Object[]{l, this, t.getMessage()});

This comment has been minimized.

Copy link
@jtnord

jtnord Apr 23, 2015

Member

so we will see the exceptions message twice?

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 23, 2015

Author Member

Yes because sometimes the exception can get separated from the message, also some people use custom formatters that drop the stack trace (bold people)

This comment has been minimized.

Copy link
@jtnord

jtnord Apr 23, 2015

Member

if you drop the stack trace in a JUL formatter you still get the message (I'm scarred by this).
If you drop logging the Throwable all together then I don't care about supporting those people and they need to learn.
JUL should log this atomically as the StreamHandler users a Writer that has internal locking semantics. Again if this doesn't happen then IMHO its a buggy 3rd party logger implementation (likely not provided by the JDK).

Anyway - I'm not passionate enough about this here to vote -1 based on this

for (Listener l : listeners.toArray(new Listener[0])) {
try {
l.onClosed(this, e);
} catch (Throwable t) {

This comment has been minimized.

Copy link
@jtnord

jtnord Apr 23, 2015

Member

As this is a listener should we be catching Errors?
Would RuntimeException not be better here?

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 23, 2015

Author Member

Debatable. e.g. if it was an OutOfMemory error it may be that closing this channel will resolve the issue anyway. Now we could do the

} catch (RuntimeException e) { 
  // log
} catch (Error e) {
  throw e;
} catch (Throwable t) {
  // log
}

dance if you feel really strongly about it... though all that will do is propagate the Error to Channel#L487 where it gets caught ant logged (incorrectly) as a failure to create the close command

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 23, 2015

Author Member

The point being that when the listener is being called, the channel has actually been closed, so we don't want to log a failure to close where it is actually a fault of the listener

This comment has been minimized.

Copy link
@jtnord

jtnord Apr 23, 2015

Member

fair enough.

@jtnord

This comment has been minimized.

Copy link
Member

commented Apr 23, 2015

👍 despite my comments on the use of JUL bits.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Apr 23, 2015

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

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 23, 2015

👍 (Other than the gratuitously complicated logger call which could have been a single line, but whatever.)

stephenc added a commit that referenced this pull request Apr 23, 2015

Merge pull request #42 from jenkinsci/safe-listener-fire
[FIXED JENKINS-28062] A Channel.Listener's onClose method that propagate...

@stephenc stephenc merged commit 846b3e0 into master Apr 23, 2015

1 check passed

Jenkins This pull request looks good
Details

@stephenc stephenc deleted the safe-listener-fire branch Apr 23, 2015

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.