From 54dfdec9606185fdffaec5aa85eea262bb375097 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 13 Apr 2018 11:16:29 +0200 Subject: [PATCH] Fix infinite loop in IntraLineLoader If edits due to rebase are present, IntraLineLoader#combineLineEdits will enter an infinite loop. As we query the intraline diff cache with a timeout check and use a fall-back behavior in case of a timeout, none of our threads got completely stuck due to that bug. On the one hand, this meant that the Gerrit server could still serve requests. On the other hand, it concealed that bug and resulted in high CPU load, which could very negatively impact the whole Gerrit server. The fix of the issue is quite simple: We increase the index counter appropriately. As this issue shows, the implementation in IntraLineLoader#combineLineEdits is quite fragile. There are better ways to achieve the same computation. Adjusting that will be left for the future. None of our tests caught that issue as the result of the fall-back mechanism was the same as the result of the real computation for all tests which were affected. For that reason, we add a test whose results differ between the regular and the fall-back mechanism. In addition, we add some tests which cover intraline edits directly. Up to now, we didn't have any tests for them. We could and should certainly add more in the future. Ideally, we would also use a timeout for all tests within RevisionDiffIT. Normally, this would be simply achieved by using the Timeout rule of JUnit. However, adding that rule breaks all tests within that class as our implementation of ConfigSuite doesn't seem to nicely interact with arbitrary JUnit rules. As the result of timed out intraline computations are cached, we need to invalidate the intraline diff cache. The high CPU load also caused a lot of PatchList cache computations to time out, for which also the fall-back result was cached. Hence, we invalidate that cache as well. Bug: Issue 8742 Change-Id: I0abf392d398320964a021099d72a4f876a73b6a6 --- .../api/revision/RevisionDiffIT.java | 107 ++++++++++++++++++ .../gerrit/server/patch/IntraLineDiffKey.java | 2 +- .../gerrit/server/patch/IntraLineLoader.java | 1 + .../gerrit/server/patch/PatchListKey.java | 2 +- .../common/ContentEntrySubject.java | 13 +++ 5 files changed, 123 insertions(+), 2 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index 7e3e0530bd..2fa55af34d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -23,6 +23,7 @@ import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toMap; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.GitUtil; @@ -82,6 +83,11 @@ public class RevisionDiffIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { + // Reduce flakiness of tests. (If tests aren't fast enough, we would use a fall-back + // computation, which might yield different results.) + baseConfig.setString("cache", "diff", "timeout", "1 minute"); + baseConfig.setString("cache", "diff_intraline", "timeout", "1 minute"); + intraline = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "intraline", false); ObjectId headCommit = testRepo.getRepository().resolve("HEAD"); @@ -1253,6 +1259,66 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); } + @Test + public void intralineEditsInNonRebaseHunksAreIdentified() throws Exception { + assume().that(intraline).isTrue(); + + Function contentModification = + fileContent -> fileContent.replace("Line 1\n", "Line one\n"); + addModifiedPatchSet(changeId, FILE_NAME, contentModification); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get(); + assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1"); + assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one"); + assertThat(diffInfo) + .content() + .element(0) + .intralineEditsOfA() + .containsExactly(ImmutableList.of(5, 1)); + assertThat(diffInfo) + .content() + .element(0) + .intralineEditsOfB() + .containsExactly(ImmutableList.of(5, 3)); + assertThat(diffInfo).content().element(0).isNotDueToRebase(); + assertThat(diffInfo).content().element(1).commonLines().hasSize(99); + } + + @Test + public void intralineEditsInRebaseHunksAreIdentified() throws Exception { + assume().that(intraline).isTrue(); + + String newFileContent = FILE_CONTENT.replace("Line 1\n", "Line one\n"); + ObjectId commit2 = addCommit(commit1, FILE_NAME, newFileContent); + + rebaseChangeOn(changeId, commit2); + Function contentModification = + fileContent -> fileContent.replace("Line 50\n", "Line fifty\n"); + addModifiedPatchSet(changeId, FILE_NAME, contentModification); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(initialPatchSetId).get(); + assertThat(diffInfo).content().element(0).linesOfA().containsExactly("Line 1"); + assertThat(diffInfo).content().element(0).linesOfB().containsExactly("Line one"); + assertThat(diffInfo) + .content() + .element(0) + .intralineEditsOfA() + .containsExactly(ImmutableList.of(5, 1)); + assertThat(diffInfo) + .content() + .element(0) + .intralineEditsOfB() + .containsExactly(ImmutableList.of(5, 3)); + assertThat(diffInfo).content().element(0).isDueToRebase(); + assertThat(diffInfo).content().element(1).commonLines().hasSize(48); + assertThat(diffInfo).content().element(2).linesOfA().containsExactly("Line 50"); + assertThat(diffInfo).content().element(2).linesOfB().containsExactly("Line fifty"); + assertThat(diffInfo).content().element(2).isNotDueToRebase(); + assertThat(diffInfo).content().element(3).commonLines().hasSize(50); + } + @Test public void closeNonRebaseHunksAreCombinedForIntralineOptimizations() throws Exception { assume().that(intraline).isTrue(); @@ -1365,6 +1431,47 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); } + @Test + public void closeNonRebaseHunksNextToRebaseHunksAreCombinedForIntralineOptimizations() + throws Exception { + assume().that(intraline).isTrue(); + + String fileContent = FILE_CONTENT.replace("Line 5\n", "{\n").replace("Line 7\n", "{\n"); + ObjectId commit2 = addCommit(commit1, FILE_NAME, fileContent); + rebaseChangeOn(changeId, commit2); + String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision; + + String newFileContent = fileContent.replace("Line 8\n", "Line eight!\n"); + ObjectId commit3 = addCommit(commit1, FILE_NAME, newFileContent); + rebaseChangeOn(changeId, commit3); + + addModifiedPatchSet( + changeId, + FILE_NAME, + content -> content.replace("Line 4\n", "Line four\n").replace("Line 6\n", "Line six\n")); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get(); + assertThat(diffInfo).content().element(0).commonLines().hasSize(3); + assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 4", "{", "Line 6"); + assertThat(diffInfo) + .content() + .element(1) + .linesOfB() + .containsExactly("Line four", "{", "Line six"); + assertThat(diffInfo).content().element(1).isNotDueToRebase(); + assertThat(diffInfo).content().element(2).commonLines().hasSize(1); + assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line 8"); + assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line eight!"); + assertThat(diffInfo).content().element(3).isDueToRebase(); + assertThat(diffInfo).content().element(4).commonLines().hasSize(92); + + Map changedFiles = + gApi.changes().id(changeId).current().files(previousPatchSetId); + assertThat(changedFiles.get(FILE_NAME)).linesInserted().isEqualTo(2); + assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(2); + } + private void assertDiffForNewFile( PushOneCommit.Result pushResult, String path, String expectedContentSideB) throws Exception { DiffInfo diff = diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java index b47bbfb050..06e2c456e8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java @@ -21,7 +21,7 @@ import org.eclipse.jgit.lib.ObjectId; @AutoValue public abstract class IntraLineDiffKey implements Serializable { - public static final long serialVersionUID = 11L; + public static final long serialVersionUID = 12L; public static IntraLineDiffKey create(ObjectId aId, ObjectId bId, Whitespace whitespace) { return new AutoValue_IntraLineDiffKey(aId, bId, whitespace); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java index d7ecc8a22d..f17f0b6856 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java @@ -271,6 +271,7 @@ class IntraLineLoader implements Callable { if (editsDueToRebase.contains(c) || editsDueToRebase.contains(n)) { // Don't combine any edits which were identified as being introduced by a rebase as we would // lose that information because of the combination. + j++; continue; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java index df5ad41982..39ebcab7ae 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java @@ -32,7 +32,7 @@ import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; public class PatchListKey implements Serializable { - public static final long serialVersionUID = 30L; + public static final long serialVersionUID = 31L; public static final ImmutableBiMap WHITESPACE_TYPES = ImmutableBiMap.of( diff --git a/gerrit-test-util/src/main/java/com/google/gerrit/extensions/common/ContentEntrySubject.java b/gerrit-test-util/src/main/java/com/google/gerrit/extensions/common/ContentEntrySubject.java index 9c9893c36f..be5b6a96d2 100644 --- a/gerrit-test-util/src/main/java/com/google/gerrit/extensions/common/ContentEntrySubject.java +++ b/gerrit-test-util/src/main/java/com/google/gerrit/extensions/common/ContentEntrySubject.java @@ -17,6 +17,7 @@ package com.google.gerrit.extensions.common; import static com.google.common.truth.Truth.assertAbout; import com.google.common.truth.FailureStrategy; +import com.google.common.truth.IterableSubject; import com.google.common.truth.StringSubject; import com.google.common.truth.Subject; import com.google.common.truth.SubjectFactory; @@ -78,4 +79,16 @@ public class ContentEntrySubject extends Subject