Skip to content

Shortlog strip console notes #4905

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.

Sign up for GitHub

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

Merged
oleg-nenashev merged 10 commits into from
Oct 8, 2020
Merged

Shortlog strip console notes #4905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8cd5ae9
Do not render parts of serialized a ConsoleNote if output begins in t…
tszmytka Aug 9, 2020
857a2ac
Do not render parts of serialized a ConsoleNote while writing non-htm…
tszmytka Aug 9, 2020
f15c856
Add positive scenario test case. Fine-tune implementation
tszmytka Aug 9, 2020
4a1f969
Render progressive output with explicitly set no preBuffer. Ensure on…
tszmytka Aug 9, 2020
bd4d86e
Revert pre-buffer functionality.
tszmytka Sep 12, 2020
be6cd7a
Push offset forward to start writing log on line beginning
tszmytka Sep 13, 2020
db7f231
Move and fine-tune tests. Avoid rendering no output in case offset fa…
tszmytka Sep 13, 2020
014b187
Merge branch 'master' into shortlog-strip-console-notes
tszmytka Sep 13, 2020
7c79230
Dont render whole log on offset on the last line. It might be problem…
tszmytka Sep 13, 2020
2e71002
Style adjustments
tszmytka Sep 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion core/src/main/java/hudson/model/Run.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import hudson.util.ProcessTree;
import hudson.util.XStream2;

import java.io.BufferedInputStream;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
Expand Down Expand Up @@ -1538,7 +1539,19 @@ public Collection<Fingerprint> getBuildFingerprints() {
* @since 1.349
*/
public void writeLogTo(long offset, @NonNull XMLOutput out) throws IOException {
getLogText().writeHtmlTo(offset, out.asWriter());
long start = offset;
if (offset > 0) {
try (BufferedInputStream bufferedInputStream = new BufferedInputStream(getLogInputStream())) {
Copy link
Member

@jglick jglick Apr 23, 2025

Choose a reason for hiding this comment

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

This is a serious performance regression for Pipeline as discovered in #10515: as of jenkinsci/workflow-job-plugin#27 this method is not intended to scale well (jenkinsci/workflow-job-plugin#27 (comment)).

if (offset == bufferedInputStream.skip(offset)) {
int r;
do {
r = bufferedInputStream.read();
start = (r == -1)? 0 : start + 1;
Copy link
Member

@oleg-nenashev oleg-nenashev Oct 2, 2020

Choose a reason for hiding this comment

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

So it will print the entire log if we hit the end of the stream, which defeats purpose of offset for large logs. Should not be a problem for real use-cases, but we might want to retain original behavior.

Suggested change
start = (r == -1)? 0 : start + 1;
start = (r == -1)? 0 : offset + 1;

Copy link
Contributor Author

@tszmytka tszmytka Oct 3, 2020

Choose a reason for hiding this comment

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

The code change is not clear to me. My idea was to advance the start position for each byte that is different than \n. What this change would do is re-set start to the value of offset + 1 multiple times - offset doesn't get updated anywhere. Can you please explain what you had in mind?

This immediately breaks 2 tests which I think are valid use cases.

} while (r != -1 && r != '\n');
}
}
}
getLogText().writeHtmlTo(start, out.asWriter());
}

/**
Expand Down
60 changes: 55 additions & 5 deletions core/src/test/java/hudson/model/RunTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@

package hudson.model;

import java.io.IOException;
import java.io.File;
import java.io.PrintWriter;
import java.io.*;
import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Locale;
import java.util.Set;
Expand All @@ -40,16 +39,21 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.console.AnnotatedLargeText;
import jenkins.model.Jenkins;
import org.apache.commons.jelly.XMLOutput;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.jvnet.hudson.test.Issue;
import org.jvnet.localizer.LocaleProvider;
import org.kohsuke.stapler.framework.io.ByteBuffer;
import org.mockito.Mockito;


public class RunTest {
private static final String SAMPLE_BUILD_OUTPUT = "Sample build output abc123.\n";

@Rule public TemporaryFolder tmp = new TemporaryFolder();

Expand Down Expand Up @@ -95,7 +99,7 @@ public String call() throws Exception {
TimeZone.setDefault(origTZ);
}
}


private List<? extends Run<?, ?>.Artifact> createArtifactList(String... paths) throws Exception {
Run r = new Run(new StubJob(), 0) {};
Expand Down Expand Up @@ -143,7 +147,7 @@ public Locale get() {
return Locale.ENGLISH;
}
});

Run r = new Run(new StubJob(), 0) {};
assertEquals("Not started yet", r.getDurationString());
r.onStartBuilding();
Expand Down Expand Up @@ -262,4 +266,50 @@ public void compareRunsFromDifferentParentsWithSameNumber() throws Exception {
assertTrue(r1.compareTo(r2) != 0);
assertTrue(treeSet.size() == 2);
}

@Test
public void willTriggerLogToStartWithNextFullLine() throws Exception {
assertWriteLogToEquals(new String(new char[2]).replace("\0", SAMPLE_BUILD_OUTPUT) + "Finished: SUCCESS.\n", 2 * SAMPLE_BUILD_OUTPUT.length() + 10);
}

@Test
public void wontPushOffsetOnRenderingFromBeginning() throws Exception {
assertWriteLogToEquals(new String(new char[5]).replace("\0", SAMPLE_BUILD_OUTPUT) + "Finished: SUCCESS.\n", 0);
}
Comment on lines +275 to +278
Copy link
Member

@jglick jglick Apr 23, 2025

Choose a reason for hiding this comment

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

Note that

@Test
public void wontPushOffsetOnRenderingFromBeginningOfLine() throws Exception {
    assertWriteLogToEquals(new String(new char[3]).replace("\0", SAMPLE_BUILD_OUTPUT) + "Finished: SUCCESS.\n", 2 * SAMPLE_BUILD_OUTPUT.length());
}

fails: it prints 2 copies, not 3 like you might expect. This is because of a “fencepost error”: when the offset happens to point to the beginning of a line (just not the first line), that line is skipped.

If this method with offset > 0 is only being used to skip 150Kb of text or whatever, the bug would hardly be noticeable.


@Test
public void willRenderNothingIfOffsetSetOnLastLine() throws Exception {
assertWriteLogToEquals("", 5 * SAMPLE_BUILD_OUTPUT.length() + 6);
}

private void assertWriteLogToEquals(String expectedOutput, long offset) throws Exception {
try (
ByteBuffer buf = new ByteBuffer();
PrintStream ps = new PrintStream(buf, true);
StringWriter writer = new StringWriter()
) {
for (int i = 0; i < 5; i++) {
ps.print(SAMPLE_BUILD_OUTPUT);
}
ps.print("Finished: SUCCESS.\n");

final Run<? extends Job<?, ?>, ? extends Run<?, ?>> r = new Run(Mockito.mock(Job.class)) {
@NonNull
@Override
public AnnotatedLargeText<?> getLogText() {
return new AnnotatedLargeText<>(buf, StandardCharsets.UTF_8, true, null);
}

@NonNull
@Override
public InputStream getLogInputStream() throws IOException {
return buf.newInputStream();
}
};
final XMLOutput xmlOutput = Mockito.mock(XMLOutput.class);
Mockito.when(xmlOutput.asWriter()).thenReturn(writer);
r.writeLogTo(offset, xmlOutput);
assertEquals(expectedOutput, writer.toString());
}
}
}