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

Adjusted TaskListener flushing in remote code #3703

Closed
wants to merge 6 commits into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 25, 2018

Amends #3563 a bit. See discussion in jenkinsci/workflow-api-plugin#81. There are several changes here:

  • A StreamTaskListener created on the master does not set the PrintStream.autoFlush flag. So why would we do so on the remote side? That merely produces a flurry of ProxyOutputStream.Flush packets. We expect that code flushes a stream when it is done with it. In fact from 2.121.x → 2.138.x for freestyle builds running Shell I see introduced flush packets, from [JENKINS-52729] Launcher.ProcStarter.stdout(TaskListener) discards remotability #3563 I presume, which this PR corrects again.
  • Conversely, RemoteLaunchCallable on the agent side produces a LocalLauncher which produces a LocalProc which uses StreamCopyThread to forward stdout/stderr of the process. Yet as far as I can tell, this never flushed its sink unless you pass the closeOut flag (which no code in core does), meaning the tail end of log output could be lost if the sink were buffered. Freestyle builds have unbuffered logs, so probably this never mattered, but for JEP-210 I am finding that some buffering for Pipeline with local log storage is useful for performance reasons, and definitely for cloud log storage it is expected. Launcher is not used in this way for durable tasks like sh, but various SynchronousNonBlockingStepExecutions (and thus also SimpleBuildSteps) use it.
  • Making sure that it is easy for a LineTransformationOutputStream to handle flush properly. Otherwise a flush on top of a stack of decorated loggers can get lost.
  • Some ConsoleNote Javadoc related to JEP-210.

Proposed changelog entries

  • Adjusted stream flushing behavior for code running remotely on agents.
  • Developer: introduced LineTransformationOutputStream.Delegating.

@jglick jglick requested a review from dwnusbaum October 25, 2018 19:32
@@ -169,7 +169,7 @@ private void writeObject(ObjectOutputStream out) throws IOException {
}

private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
out = new PrintStream((OutputStream)in.readObject(),true);
out = new PrintStream((OutputStream)in.readObject(), false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect that code flushes a stream when it is done with it

That expectation seems reasonable and preferable to the current behavior, but given that this code has been unmodified since 2007 I am a little concerned that this may break things. Should be easy enough to fix if it does cause issues, but I do not have a good sense of whether there may be widespread impact.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that this code has been unmodified since 2007 I am a little concerned that this may break things

Indeed. Of course that is true of a lot of Jenkins code.

@jglick jglick requested a review from dwnusbaum October 25, 2018 23:23
@jglick jglick changed the title Adjusted flushing behavior of remote code using TaskListener Adjusted TaskListener flushing in remote code Oct 25, 2018
@jglick jglick added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Oct 26, 2018
@jglick
Copy link
Member Author

jglick commented Oct 26, 2018

I want to do some testing of this and sleep on a bit more before pushing into releases.

…otator had neglected to implement flush properly.
jglick added a commit to jglick/timestamper-plugin that referenced this pull request Oct 26, 2018
jglick added a commit to jglick/ant-plugin that referenced this pull request Oct 26, 2018
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes look good to me. Do you plan to add any tests here, or just do some manual testing of various affected scenarios?

@jglick
Copy link
Member Author

jglick commented Oct 30, 2018

Do you plan to add any tests here

TBD.

or just do some manual testing

Have done some sanity checks so far, but deserves more.

cyrille-leclerc pushed a commit to jenkinsci/pipeline-maven-plugin that referenced this pull request Oct 31, 2018
@jglick
Copy link
Member Author

jglick commented Nov 1, 2018

Note to self: need to document expected caller usage of remoted TaskListener.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jglick
Copy link
Member Author

jglick commented Apr 2, 2019

This just turned into a grab bag of JEP-210-related patches which would be better refiled separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-hold This pull request depends on another event/release, and it cannot be merged right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants