Define OutputStreamTaskListener & close BufferedBuildListener
#294
Conversation
| Logger.getLogger(OutputStreamTaskListener.class.getName()).warning(() -> "Unexpected PrintStream subclass " + ps.getClass().getName() + " which might override write(…); error handling is degraded unless OutputStreamTaskListener is used: " + listener.getClass().getName()); | ||
| return ps; | ||
| } | ||
| if (Runtime.version().compareToIgnoreOptional(Runtime.Version.parse("17")) >= 0) { | ||
| Logger.getLogger(OutputStreamTaskListener.class.getName()).warning(() -> "On Java 17+ error handling is degraded unless OutputStreamTaskListener is used: " + listener.getClass().getName()); |
There was a problem hiding this comment.
Should we add a stack trace to these messages? Otherwise I don't really see how a Jenkins developer would be able to track down the problematic code.
There was a problem hiding this comment.
The problem is just with the named class. A stack trace would not really add anything important.
There was a problem hiding this comment.
But the interesting thing is where that class is being used, right? E.g. If I see StreamTaskListener this message is unhelpful. Or do you only think this will only happen for custom listener implementations where the class name makes the use site obvious?
There was a problem hiding this comment.
It should only happen for custom TaskListener implementations (this is for Pipeline builds where StreamTaskListener should not be used), and the use site is beside the point—either the class implements the new interface or it does not.
There was a problem hiding this comment.
It should only happen for custom TaskListener implementations (this is for Pipeline builds where StreamTaskListener should not be used)
Ok, probably fine then.
either the class implements the new interface or it does not.
Sure, but it was not obvious to me what classes we would expect here. If StreamTaskListener or LogTaskListener is possible then the message is not helpful and my concern was that it might cause confusing log spam. For example if we hit https://github.com/jenkinsci/workflow-durable-task-step-plugin/blob/1f55bd46852f56767bd2d664fa055deaa98c88cf/src/main/java/org/jenkinsci/plugins/workflow/steps/durable_task/DurableTaskStep.java#L433 (seems like an obscure case) then is this warning reachable and would it be logged repeatedly? It isn't obvious to me.
There was a problem hiding this comment.
In that case probably, but something is already pretty broken then. The warning could be limited to classes defined in plugins, and/or for that error handler we could make a special implementation. I can file a follow-up to refine a little bit.
There was a problem hiding this comment.
Ok, it does not seem particularly important given your explanation, it just wasn't totally clear to me what kinds of scenarios we should anticipate.
| import org.kohsuke.accmod.restrictions.Beta; | ||
|
|
||
| /** | ||
| * {@link TaskListener} which can directly return an {@link OutputStream} not wrapped in a {@link PrintStream}. |
There was a problem hiding this comment.
Logging improvements needed for jenkinsci/workflow-durable-task-step-plugin#323.