Skip to content

Commit

Permalink
SLI-1203 Fix LiveFindingCache deadlock
Browse files Browse the repository at this point in the history
  • Loading branch information
@damien-urruty-sonarsource
damien-urruty-sonarsource committed Dec 7, 2023
1 parent 1bdc21b commit cbd4a04
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 46 deletions.
25 changes: 11 additions & 14 deletions src/main/java/org/sonarlint/intellij/finding/persistence/LiveFindingCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -51,12 +52,12 @@ public LiveFindingCache(Project project, FindingPersistence<T> persistence) {
this.project = project;
this.persistence = persistence;
this.maxEntries = maxEntries;
this.cache = new LimitedSizeLinkedHashMap();
this.cache = Collections.synchronizedMap(new LimitedSizeLinkedHashMap());
}

public synchronized void replaceFindings(Map<VirtualFile, Collection<T>> newFindingsPerFile) {
public void replaceFindings(Map<VirtualFile, Collection<T>> newFindingsPerFile) {
cache.putAll(newFindingsPerFile);
flushAll();
flushAll(newFindingsPerFile);
}

/**
Expand Down Expand Up @@ -119,26 +120,22 @@ public Collection<Trackable> getPreviousFindings(VirtualFile file) {
* Read findings from a file that are cached. On cache miss, it won't fallback to the persistent store.
*/
@CheckForNull
public synchronized Collection<T> getLive(VirtualFile virtualFile) {
public Collection<T> getLive(VirtualFile virtualFile) {
var liveFindings = cache.get(virtualFile);
if (liveFindings != null) {
// Create a copy to avoid concurrent modification issues
// Create a copy to avoid concurrent modification issues later
return new ArrayList<>(liveFindings);
}
return null;
}

public synchronized void insertFinding(VirtualFile virtualFile, T finding) {
cache.computeIfAbsent(virtualFile, f -> new ArrayList<>()).add(finding);
}

/**
* Flushes all cached entries to disk.
* Flushes all provided entries to disk.
* It does not clear the cache.
*/
private void flushAll() {
private void flushAll(Map<VirtualFile, Collection<T>> newFindingsPerFile) {
SonarLintConsole.get(project).debug("Persisting all findings");
cache.forEach((virtualFile, liveFindings) -> {
newFindingsPerFile.forEach((virtualFile, liveFindings) -> {
if (virtualFile.isValid()) {
var key = createKey(virtualFile);
if (key != null) {
Expand All @@ -155,12 +152,12 @@ private void flushAll() {
/**
* Clear cache and underlying persistent store
*/
public synchronized void clear() {
public void clear() {
persistence.clear();
cache.clear();
}

public synchronized boolean contains(VirtualFile virtualFile) {
public boolean contains(VirtualFile virtualFile) {
return getLive(virtualFile) != null;
}

Expand Down
1 change: 1 addition & 0 deletions src/main/resources/META-INF/plugin.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@

<change-notes><![CDATA[
<ul>
<li>10.1.1 - Fix a potential deadlock and UI freeze.</li>
<li>10.1 - 9 new Java rules related to Spring Boot, 1 new quick fix. 17 new accessibility rules for JSX. 6 new Kotlin rules. Support for Python 3.12 syntax, add 5 related rules, add 3 new Security Hotspots. 2 new C++ MISRA 2023 rules, support for Docker toolchain in CLion. Support for .NET 8 in Rider. Bug fixes, fewer FPs and improvements for many languages.</li>
<li>10.0.1 - Fix an error when opening an issue from SonarQube. Fix potential UI-freeze.</li>
<li>10.0 - Raise minimal supported version to 2022.3.1 and Java 17. Allow to reopen taint vulnerabilities. Handle "Open in IDE" requests from SonarQube. Synchronize issues and Security Hotspots in real-time from SonarCloud. 5 new rules for Python. 6 new rules for Java. 29 new rules for JavaScript. Bug fixes, fewer FPs and improvements for many languages.</li>
Expand Down
46 changes: 14 additions & 32 deletions src/test/java/org/sonarlint/intellij/finding/persistence/LiveFindingCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

Expand All @@ -59,54 +58,38 @@ void before() {
}

@Test
void should_save_and_read_cache_only() {
void should_save_and_read_cache_only() throws IOException {
var file = myFixture.copyFileToProject("foo.php", "foo.php");
var issue = createTestIssue("r1");
cache.insertFinding(file, issue);
cache.replaceFindings(Map.of(file, List.of(issue)));

assertThat(cache.contains(file)).isTrue();
assertThat(cache.getLive(file)).containsOnly(issue);

assertThat(cache.contains(myFixture.copyFileToProject("foo.php", "foo2.php"))).isFalse();

verifyNoInteractions(store);
}

@Test
void should_not_fallback_persistence() {
var file = myFixture.copyFileToProject("foo.php", "foo.php");
var issue1 = createTestIssue("r1");

cache.insertFinding(file, issue1);

var cacheMiss = myFixture.copyFileToProject("foo.php", "foo2.php");
assertThat(cache.getLive(cacheMiss)).isNull();

verifyNoInteractions(store);
verify(store).save("foo.php", List.of(issue));
}

@Test
void should_flush_if_full() throws IOException {
var issue1 = createTestIssue("r1");
var file0 = myFixture.copyFileToProject("foo.php", "foo0.php");
cache.insertFinding(file0, issue1);
cache.replaceFindings(Map.of(file0, List.of(issue1)));
var file1 = myFixture.copyFileToProject("foo.php", "foo1.php");
cache.insertFinding(file1, issue1);
cache.replaceFindings(Map.of(file1, List.of(issue1)));

for (var i = 2; i < MAX_ENTRIES_FOR_TEST; i++) {
var file = myFixture.copyFileToProject("foo.php", "foo" + i + ".php");
cache.insertFinding(file, issue1);
cache.replaceFindings(Map.of(file, List.of(issue1)));
}

// oldest access should be foo1.php after this
assertThat(cache.getLive(file0)).containsOnly(issue1);

verifyNoInteractions(store);

var file = myFixture.copyFileToProject("foo.php", "anotherfile.php");
cache.insertFinding(file, issue1);
cache.replaceFindings(Map.of(file, List.of(issue1)));

verify(store).save(eq("foo1.php"), anyCollection());
assertThat(cache.getLive(file0)).containsOnly(issue1);
// File1 has been flushed
assertThat(cache.getLive(file1)).isNull();
Expand All @@ -116,7 +99,7 @@ void should_flush_if_full() throws IOException {
void should_clear_store() {
var issue1 = createTestIssue("r1");
var file0 = myFixture.copyFileToProject("foo.php", "foo0.php");
cache.insertFinding(file0, issue1);
cache.replaceFindings(Map.of(file0, List.of(issue1)));

cache.clear();
verify(store).clear();
Expand All @@ -141,7 +124,6 @@ void handle_error_flush() throws IOException {

var issue1 = createTestIssue("r1");
var file0 = myFixture.copyFileToProject("foo.php", "foo0.php");
cache.insertFinding(file0, issue1);
cache.replaceFindings(Map.of(file0, List.of(issue1)));

assertThat(((SonarLintConsoleTestImpl)getConsole()).getLastMessage()).contains("Cannot flush issues");
Expand All @@ -154,11 +136,11 @@ void error_remove_eldest() throws IOException {
var issue1 = createTestIssue("r1");
for (var i = 0; i < MAX_ENTRIES_FOR_TEST; i++) {
var file = myFixture.copyFileToProject("foo.php", "foo" + i + ".php");
cache.insertFinding(file, issue1);
cache.replaceFindings(Map.of(file, List.of(issue1)));
}

var extraFile = myFixture.copyFileToProject("foo.php", "another.php");
assertThrows(IllegalStateException.class, () -> cache.insertFinding(extraFile, issue1));
assertThrows(IllegalStateException.class, () -> cache.replaceFindings(Map.of(extraFile, List.of(issue1))));
}

@Test
Expand All @@ -168,13 +150,13 @@ void testConcurrentAccess() throws Exception {

Runnable r = () -> {
LiveIssue issue1 = createTestIssue("r1");
cache.insertFinding(file1, issue1);
cache.replaceFindings(Map.of(file1, List.of(issue1)));
LiveIssue issue2 = createTestIssue("r2");
cache.insertFinding(file1, issue2);
cache.replaceFindings(Map.of(file1, List.of(issue2)));
LiveIssue issue3 = createTestIssue("r3");
cache.insertFinding(file2, issue3);
cache.replaceFindings(Map.of(file2, List.of(issue3)));
LiveIssue issue4 = createTestIssue("r4");
cache.insertFinding(file2, issue4);
cache.replaceFindings(Map.of(file2, List.of(issue4)));
Collection<LiveIssue> live = cache.getLive(file1);
if (live != null) {
assertThat(live).extracting(LiveIssue::getRuleKey).isSubsetOf("r1", "r2");
Expand Down

0 comments on commit cbd4a04

Please sign in to comment.