[JENKINS-28062] Channel listener onClose propagated exceptions #1669
Conversation
…ates an exception is a sign of a bad Launcher not a problem closing the channel.
| LogRecord lr = new LogRecord(Level.SEVERE, | ||
| "Launcher {0}'s afterDisconnect method propagated an exception when {1}'s connection was closed: {2}"); | ||
| lr.setThrown(t); | ||
| lr.setParameters(new Object[]{launcher, SlaveComputer.this, t.getMessage()}); |
There was a problem hiding this comment.
you will see the exception message twice in the message.
There was a problem hiding this comment.
I wonder if it not the time to wrap java.util.logging.Logger into more convenient interface or perhaps incorporate such wrapper if exists already.
There was a problem hiding this comment.
@jtnord you assume that people are not supressing stack traces in their logs
@olivergondza maybe... but that would need to be in core and then you'd need to wait for core to provide it... in which case I'd much rather just switch all of core to slf4j rather than re-invent the wheel
There was a problem hiding this comment.
@stephenc FWIW we have bundled slf4j for a long time now as plugins use it!
There was a problem hiding this comment.
@stephenc those that suppress stack traces are ok. (you can print the throwable's getMessage() without the stack trace)
those that suppress throwables entirely are stoopid - and see my comments on the related fix in remoting.
If we don't have a stack trace fixing things becomes very hard, and a message doesn't help anyone.
We should not be trying to help stupid by making things look worse for normal users.
If we want to help stoopid we should educate stoopid on why this is stoopid... (and yes I have done this in the past - its slow progress but the only real way)
Having the message printed twice to me is a sign of a junior developer and doesn't look good.
I know you don't fit into this category...
I don't care strongly enough to vote -1 based on this - but this style is not followed in the rest of Jenkins or it's plugins so it's really a worthless effort IMHO.
There was a problem hiding this comment.
Plain old
logger.log(Level.SEVERE, "Launcher " + launcher + "'s afterDisconnect method propagated an exception when " + SlaveComputer.this + "'s connection was closed", t)would suffice. The Jenkins LogManager offers no facility for suppressing stack traces so there is no reason to worry about it. And delaying string concatenation is a pointless optimization when SEVERE is logged by default.
There was a problem hiding this comment.
@jglick but then somebody decided to change logging level and it will be slow logging. Better do good code as example for plugin developers
There was a problem hiding this comment.
then somebody decided to change logging level and it will be slow logging
So under a condition I have never seen in the wild (which would probably require some Jenkins core patches to even implement, since /log/all is hardcoded to log everything at INFO or above), this catch statement which is probably encountered infrequently will take another few microseconds. Hardly justification for longer and harder-to-read code.
There was a problem hiding this comment.
Code becomes complex when ancient classes/methods not removed for 10 years and library forked with single patches, even java itself already cutting compatibility.
logger.log(Level.SEVERE, "Launcher {0} afterDisconnect method failed: \" {1} \". {2} connection was closed", \n
new Object[] {launcher, t.getMessage(), SlaveComputer.this});
And not sure that SlaveComputer.this is good description.
|
👍 despite my comments on the (ab)use of JUL. |
jglick
changed the title
| launcher.afterDisconnect(SlaveComputer.this, taskListener); | ||
| } catch (Throwable t) { | ||
| LogRecord lr = new LogRecord(Level.SEVERE, | ||
| "Launcher {0}'s afterDisconnect method propagated an exception when {1}'s connection was closed: {2}"); |
There was a problem hiding this comment.
Printing the ComputerLauncher.toString is probably senseless since any interesting information about the implementation would be in the stack trace, and toString is unlikely to be overridden from Object. Printing the SlaveComputer.toString is definitely senseless since this is not overridden; you want to use nodeName (or indirectly via getName).
|
Close enough. 👍 |
[JENKINS-28062] Channel listener onClose propagated exceptions
JENKINS-28062
A Launcher.afterDisconnect() method that propagates an exception is a sign of a bad Launcher not a problem closing the channel.
related to jenkinsci/remoting#42 but helps us get closer to identifying the root cause of the issue
@reviewbybees