Skip to content

Commit

Permalink
Do not permanently close the log stream in FileLogStorage if an inter…
Browse files Browse the repository at this point in the history
…rupted thread attempts to write to it
  • Loading branch information
dwnusbaum committed Jul 18, 2023
1 parent 1679fa2 commit 7191b33
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -126,17 +126,28 @@ private void checkId(String id) throws IOException {
assert Thread.holdsLock(this);
if (!Objects.equals(id, lastId)) {
bos.flush();
long pos = os.getChannel().position();
if (id == null) {
indexOs.write(pos + "\n");
} else {
indexOs.write(pos + " " + id + "\n");
// FileChannel methods such as position() close the channel (and the stream it was derived from) if the
// current thread is interrupted. We must not allow interruption on some arbitrary thread attempting to
// write to the log to close the stream irrevocably, so we clear the interrupt status before using the
// channel and then restore the interrupt status afterwards.
boolean interrupted = Thread.interrupted();
try {
long pos = os.getChannel().position();
if (id == null) {
indexOs.write(pos + "\n");
} else {
indexOs.write(pos + " " + id + "\n");
}
// Could call FileChannel.force(true) like hudson.util.FileChannelWriter does for AtomicFileWriter,
// though making index-log writes slower is likely a poor tradeoff for slightly more reliable log display,
// since logs are often never read and this is transient data rather than configuration or valuable state.
indexOs.flush();
lastId = id;
} finally {
if (interrupted) {
Thread.currentThread().interrupt();
}
}
// Could call FileChannel.force(true) like hudson.util.FileChannelWriter does for AtomicFileWriter,
// though making index-log writes slower is likely a poor tradeoff for slightly more reliable log display,
// since logs are often never read and this is transient data rather than configuration or valuable state.
indexOs.flush();
lastId = id;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,18 @@ public class FileLogStorageTest extends LogStorageTestBase {
assertOverallLog(0, lines("stuff"), true);
}

@Test public void interruptionDoesNotCloseStream() throws Exception {
LogStorage ls = createStorage();
TaskListener overall = ls.overallListener();
overall.getLogger().println("overall 1");
Thread.currentThread().interrupt();
TaskListener stepLog = ls.nodeListener(new MockNode("1"));
stepLog.getLogger().println("step 1");
assertTrue(Thread.interrupted());
close(stepLog);
overall.getLogger().println("overall 2");
close(overall);
assertOverallLog(0, lines("overall 1", "<span class=\"pipeline-node-1\">step 1", "</span>overall 2"), true);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -350,12 +350,12 @@ protected String lines(CharSequence... lines) {
return String.join(System.lineSeparator(), lines) + System.lineSeparator();
}

private static class MockNode extends FlowNode {
protected static class MockNode extends FlowNode {
MockNode(String id) {super(new MockFlowExecution(), id);}
@Override protected String getTypeDisplayName() {return null;}
}

private static class MockFlowExecution extends FlowExecution {
protected static class MockFlowExecution extends FlowExecution {
@Override
public void start() {
throw new UnsupportedOperationException();
Expand Down

0 comments on commit 7191b33

Please sign in to comment.