Skip to content

Conversation

@jglick
Copy link
Member

@jglick jglick commented Nov 5, 2018

Fix to #112 discovered by @kshultzCB.

@jglick jglick requested review from dwnusbaum and svanoort November 5, 2018 19:18
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.

Looks good to me assuming @kshultzCB confirms it fixes the issue he was seeing.

@jglick
Copy link
Member Author

jglick commented Nov 5, 2018

more CI problems

@jglick jglick closed this Nov 5, 2018
@jglick jglick reopened this Nov 5, 2018
Copy link
Member

@svanoort svanoort left a comment

Choose a reason for hiding this comment

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

This needs some form of unit test

// Similar to writeWholeLogTo but terminates even if !logText.complete:
long pos = 0;
while (true) {
long pos2 = getLogText().writeLogTo(pos, os);
Copy link
Member

Choose a reason for hiding this comment

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

Interesting possibility: what if the log is being generated faster than it can be written to output?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I guess you are going to be swamped with output. Not much we can do about that.

@dwnusbaum
Copy link
Member

dwnusbaum commented Nov 5, 2018

getLogInputStream (a few lines up in the diff) has the same problem, right?

@svanoort svanoort merged commit a540fc3 into jenkinsci:master Nov 6, 2018
@jglick jglick deleted the doConsoleText-2 branch November 6, 2018 18:28
jglick added a commit to jglick/jenkins that referenced this pull request Nov 8, 2018
jglick added a commit to jglick/jenkins that referenced this pull request Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants