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

[JENKINS-43199, JENKINS-45057] Reliably close build log file #2954

Merged
merged 2 commits into from Aug 3, 2017

Conversation

5 participants
@jglick
Copy link
Member

commented Jul 31, 2017

Simpler alternative to #2953.

  • JENKINS-43199, JENKINS-45057 - Bug - Reliably close build log file when using chained Build Listeners

@reviewbybees

@jglick jglick requested a review from stephenc Jul 31, 2017

@reviewbybees

This comment has been minimized.

Copy link

commented Jul 31, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@@ -1787,6 +1789,13 @@ protected final void execute(@Nonnull RunExecution job) {
listener.finished(result);
listener.closeQuietly();
}
if (logger != null) {

This comment has been minimized.

Copy link
@stephenc

stephenc Aug 1, 2017

Member

if listener.finished(result) throws an exception, the logger will not be closed

This comment has been minimized.

Copy link
@jglick

jglick Aug 2, 2017

Author Member

True, but that was already the case; this patch is merely improving the behavior when listener.close() does not call logger.close().

@alexanderrtaylor
Copy link
Contributor

left a comment

This LGTM 🐝

I think @stephenc's comments are valid but that is not a net new issue(it would not be closed properly before the new changes as well)

LOGGER.log(Level.SEVERE, "Failed to rotate log",e);
}
LOGGER.log(Level.SEVERE, "Failed to rotate log",e);
}

This comment has been minimized.

Copy link
@jglick

jglick Aug 2, 2017

Author Member

#1790 introduced tabs.

@oleg-nenashev
Copy link
Member

left a comment

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

I am integrating it in order to get the patch in 2.60.3 RC

@oleg-nenashev oleg-nenashev merged commit a0a55d1 into jenkinsci:master Aug 3, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@oleg-nenashev oleg-nenashev changed the title [JENKINS-43199] Reliably close build log file [JENKINS-43199, JENKINS-45057] Reliably close build log file Aug 3, 2017

@jglick jglick deleted the jglick:leak-JENKINS-43199 branch Aug 3, 2017

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.