Skip to content

Commit 2b36a82

Browse files
nquinquenelnquinquenel
authored
SLCORE-1509 Cancellation should include queued task that are not started (#1399)
1 parent 7f5c84a commit 2b36a82

File tree

17 files changed

+178
-30
lines changed

17 files changed

+178
-30
lines changed

‎.sonarlint/connectedMode.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,4 @@
22
"sonarQubeUri": "https://next.sonarqube.com/sonarqube",
33
"projectKey": "org.sonarsource.sonarlint.core:sonarlint-core-parent"
44
}
5+

‎backend/analysis-engine/src/main/java/org/sonarsource/sonarlint/core/analysis/AnalysisQueue.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,17 @@ private Optional<QueuedCommand> pollNextReadyCommand() {
8585
// cannot use iterator as priority order is not guaranteed
8686
while (!queue.isEmpty()) {
8787
var candidateCommand = queue.poll();
88-
if (candidateCommand.command.isReady()) {
89-
queue.addAll(commandsToKeep);
90-
return Optional.of(candidateCommand);
88+
if (candidateCommand.command.shouldCancelQueue()) {
89+
candidateCommand.command.cancel();
90+
LOG.debug("Not picking next command {}, is canceled", candidateCommand.command);
91+
} else {
92+
if (candidateCommand.command.isReady()) {
93+
queue.addAll(commandsToKeep);
94+
return Optional.of(candidateCommand);
95+
}
96+
LOG.debug("Not picking next command {}, is not ready", candidateCommand.command);
97+
commandsToKeep.add(candidateCommand);
9198
}
92-
LOG.debug("Not picking next command {}, is not ready", candidateCommand.command);
93-
commandsToKeep.add(candidateCommand);
9499
}
95100
queue.addAll(commandsToKeep);
96101
return Optional.empty();

‎backend/analysis-engine/src/main/java/org/sonarsource/sonarlint/core/analysis/AnalysisScheduler.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,8 @@ public void post(Command command) {
9999
return;
100100
}
101101
var currentCommand = executingCommand.get();
102-
if (currentCommand != null && command.shouldCancel(currentCommand)) {
103-
LOG.debug("Cancelling execution of executing command");
102+
if (currentCommand != null && command.shouldCancelPost(currentCommand)) {
103+
LOG.debug("Cancelling queuing of command");
104104
currentCommand.cancel();
105105
}
106106
LOG.debug("Posting command from Scheduler to queue: " + command);

‎backend/analysis-engine/src/main/java/org/sonarsource/sonarlint/core/analysis/command/AnalyzeCommand.java‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ public AnalyzeCommand(String moduleKey, UUID analysisId, TriggerType triggerType
7777
Set<URI> files, Map<String, String> extraProperties) {
7878
this(moduleKey, analysisId, triggerType, configurationSupplier, issueListener, trace, cancelMonitor, taskManager, analysisStarted, isReadySupplier, files, extraProperties,
7979
new CompletableFuture<>());
80-
8180
}
8281

8382
public AnalyzeCommand(String moduleKey, UUID analysisId, TriggerType triggerType, Supplier<AnalysisConfiguration> configurationSupplier, Consumer<Issue> issueListener,
@@ -96,6 +95,8 @@ public AnalyzeCommand(String moduleKey, UUID analysisId, TriggerType triggerType
9695
this.files = files;
9796
this.extraProperties = extraProperties;
9897
this.futureResult = futureResult;
98+
99+
taskManager.trackNewTask(analysisId, cancelMonitor);
99100
}
100101

101102
@Override
@@ -127,7 +128,7 @@ public Map<String, String> getExtraProperties() {
127128
public void execute(ModuleRegistry moduleRegistry) {
128129
try {
129130
var configuration = configurationSupplier.get();
130-
taskManager.runTask(moduleKey, analysisId, "Analyzing " + pluralize(configuration.inputFiles().size(), "file"), null, true, true,
131+
taskManager.runExistingTask(moduleKey, analysisId, "Analyzing " + pluralize(configuration.inputFiles().size(), "file"), null, true, true,
131132
indicator -> execute(moduleRegistry, indicator, configuration), cancelMonitor);
132133
} catch (Exception e) {
133134
handleAnalysisFailed(e);
@@ -254,21 +255,33 @@ public AnalyzeCommand mergeWith(AnalyzeCommand otherNewerAnalyzeCommand) {
254255

255256
@Override
256257
public void cancel() {
257-
cancelMonitor.cancel();
258-
futureResult.cancel(true);
258+
if (!cancelMonitor.isCanceled()) {
259+
cancelMonitor.cancel();
260+
}
261+
if (!futureResult.isCancelled()) {
262+
futureResult.cancel(true);
263+
}
259264
}
260265

261266
@Override
262-
public boolean shouldCancel(Command executingCommand) {
267+
public boolean shouldCancelPost(Command executingCommand) {
263268
if (!(executingCommand instanceof AnalyzeCommand analyzeCommand)) {
264269
return false;
265270
}
271+
if (cancelMonitor.isCanceled() || futureResult.isCancelled()) {
272+
return true;
273+
}
266274
var triggerTypesMatch = getTriggerType() == analyzeCommand.getTriggerType();
267275
var filesMatch = Objects.equals(getFiles(), analyzeCommand.getFiles());
268276
var extraPropertiesMatch = Objects.equals(getExtraProperties(), analyzeCommand.getExtraProperties());
269277
return triggerTypesMatch && filesMatch && extraPropertiesMatch;
270278
}
271279

280+
@Override
281+
public boolean shouldCancelQueue() {
282+
return cancelMonitor.isCanceled() || futureResult.isCancelled();
283+
}
284+
272285
@CheckForNull
273286
public Trace getTrace() {
274287
return trace;

‎backend/analysis-engine/src/main/java/org/sonarsource/sonarlint/core/analysis/command/Command.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,11 @@ public void cancel() {
4040
// most commands are not cancelable
4141
}
4242

43-
public boolean shouldCancel(Command executingCommand) {
43+
public boolean shouldCancelPost(Command executingCommand) {
44+
return false;
45+
}
46+
47+
public boolean shouldCancelQueue() {
4448
return false;
4549
}
4650
}

‎backend/analysis-engine/src/main/java/org/sonarsource/sonarlint/core/analysis/command/UnregisterModuleCommand.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public String getModuleKey() {
3838
}
3939

4040
@Override
41-
public boolean shouldCancel(Command executingCommand) {
41+
public boolean shouldCancelPost(Command executingCommand) {
4242
return executingCommand instanceof AnalyzeCommand analyzeCommand && analyzeCommand.getModuleKey().equals(moduleKey);
4343
}
4444
}

‎backend/analysis-engine/src/test/java/org/sonarsource/sonarlint/core/analysis/AnalysisQueueTest.java‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,15 @@
2424
import java.util.UUID;
2525
import org.junit.jupiter.api.Test;
2626
import org.junit.jupiter.api.extension.RegisterExtension;
27+
import org.sonarsource.sonarlint.core.analysis.api.TriggerType;
2728
import org.sonarsource.sonarlint.core.analysis.command.AnalyzeCommand;
2829
import org.sonarsource.sonarlint.core.analysis.command.RegisterModuleCommand;
2930
import org.sonarsource.sonarlint.core.commons.log.SonarLintLogTester;
31+
import org.sonarsource.sonarlint.core.commons.progress.SonarLintCancelMonitor;
32+
import org.sonarsource.sonarlint.core.commons.progress.TaskManager;
3033

3134
import static org.assertj.core.api.Assertions.assertThat;
35+
import static org.mockito.Mockito.mock;
3236

3337
class AnalysisQueueTest {
3438
@RegisterExtension
@@ -37,12 +41,31 @@ class AnalysisQueueTest {
3741
@Test
3842
void it_should_prioritize_register_module_commands_over_analyses() throws InterruptedException {
3943
var analysisQueue = new AnalysisQueue();
40-
analysisQueue.post(new AnalyzeCommand(null, UUID.randomUUID(), null, null, null, null, null, null, null, () -> true, Set.of(), Map.of()));
44+
var taskManager = mock(TaskManager.class);
45+
analysisQueue.post(new AnalyzeCommand(null, UUID.randomUUID(), null, null, null, null, null, taskManager, null, () -> true, Set.of(), Map.of()));
4146
var registerModuleCommand = new RegisterModuleCommand(null);
4247
analysisQueue.post(registerModuleCommand);
4348

4449
var command = analysisQueue.takeNextCommand();
4550

4651
assertThat(command).isEqualTo(registerModuleCommand);
4752
}
53+
54+
@Test
55+
void it_should_not_queue_a_canceled_command() throws InterruptedException {
56+
var canceledProgressMonitor = new SonarLintCancelMonitor();
57+
var progressMonitor = new SonarLintCancelMonitor();
58+
var analysisQueue = new AnalysisQueue();
59+
var taskManager = mock(TaskManager.class);
60+
var canceledCommand = new AnalyzeCommand("1", UUID.randomUUID(), TriggerType.FORCED, null, null, null, canceledProgressMonitor, taskManager, null, () -> true, Set.of(), Map.of());
61+
var command = new AnalyzeCommand("2", UUID.randomUUID(), TriggerType.FORCED, null, null, null, progressMonitor, taskManager, null, () -> true, Set.of(), Map.of());
62+
canceledProgressMonitor.cancel();
63+
analysisQueue.post(canceledCommand);
64+
analysisQueue.post(command);
65+
66+
var nextCommand = analysisQueue.takeNextCommand();
67+
68+
assertThat(nextCommand).isEqualTo(command);
69+
}
70+
4871
}

‎backend/analysis-engine/src/test/java/org/sonarsource/sonarlint/core/analysis/command/AnalyzeCommandTest.java‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
*/
2020
package org.sonarsource.sonarlint.core.analysis.command;
2121

22+
import java.net.URI;
2223
import java.time.Duration;
2324
import java.time.temporal.ChronoUnit;
2425
import java.util.Map;
@@ -68,4 +69,64 @@ void it_should_rethrow_an_exception_when_stopping_a_transient_module_container()
6869
.withMessage("Kaboom");
6970
}
7071

72+
@Test
73+
void it_should_cancel_posting_command() throws Exception {
74+
var files = Set.of(new URI("file:///test1"));
75+
var props = Map.of("a", "b");
76+
var cmd1 = newAnalyzeCommand(files, props);
77+
var cmd2 = newAnalyzeCommand(files, props);
78+
79+
assertThat(cmd1.shouldCancelPost(cmd2)).isTrue();
80+
}
81+
82+
@Test
83+
void it_should_cancel_posting_command_if_canceled() throws Exception {
84+
var cmd1 = newAnalyzeCommand(Set.of(new URI("file:///test1")), Map.of());
85+
var cmd2 = newAnalyzeCommand(Set.of(new URI("file:///test2")), Map.of());
86+
cmd1.cancel();
87+
88+
assertThat(cmd1.shouldCancelPost(cmd2)).isTrue();
89+
}
90+
91+
@Test
92+
void it_should_not_cancel_when_files_are_different() throws Exception {
93+
var cmd1 = newAnalyzeCommand(Set.of(new URI("file:///test1")), Map.of());
94+
var cmd2 = newAnalyzeCommand(Set.of(new URI("file:///test2")), Map.of());
95+
96+
assertThat(cmd1.shouldCancelPost(cmd2)).isFalse();
97+
}
98+
99+
100+
@Test
101+
void if_should_cancel_task_in_queue_when_canceled() {
102+
var cmd = newAnalyzeCommand(Set.of(), Map.of());
103+
cmd.cancel();
104+
105+
assertThat(cmd.shouldCancelQueue()).isTrue();
106+
}
107+
108+
@Test
109+
void it_should_not_cancel_task_in_queue_if_not_canceled() {
110+
var cmd = newAnalyzeCommand(Set.of(), Map.of());
111+
112+
assertThat(cmd.shouldCancelQueue()).isFalse();
113+
}
114+
115+
private static AnalyzeCommand newAnalyzeCommand(Set<URI> files, Map<String, String> extraProps) {
116+
return new AnalyzeCommand(
117+
"moduleKey",
118+
UUID.randomUUID(),
119+
TriggerType.FORCED,
120+
() -> AnalysisConfiguration.builder().addInputFiles().build(),
121+
issue -> {},
122+
null,
123+
new SonarLintCancelMonitor(),
124+
new TaskManager(),
125+
inputFiles -> {},
126+
() -> true,
127+
files,
128+
extraProps
129+
);
130+
}
131+
71132
}

‎backend/analysis-engine/src/test/java/org/sonarsource/sonarlint/core/analysis/mediumtests/AnalysisSchedulerMediumTests.java‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,13 +224,13 @@ def foo():
224224
var issues2 = new ArrayList<>();
225225
var analyzeCommand1 = new AnalyzeCommand("moduleKey", UUID.randomUUID(), TriggerType.FORCED,
226226
() -> analysisConfig, issues1::add, null, progressMonitor, TASK_MANAGER,
227-
NO_OP_ANALYSIS_STARTED_CONSUMER, ANALYSIS_READY_SUPPLIER, Set.of(), Map.of());
227+
NO_OP_ANALYSIS_STARTED_CONSUMER, ANALYSIS_READY_SUPPLIER, Set.of(), Map.of("a", "1"));
228228
var throwingCommand = new AnalyzeCommand("moduleKey", UUID.randomUUID(), TriggerType.FORCED,
229229
() -> analysisConfig, NO_OP_ISSUE_LISTENER, null, progressMonitor, TASK_MANAGER,
230-
NO_OP_ANALYSIS_STARTED_CONSUMER, throwingSupplier, Set.of(), Map.of());
230+
NO_OP_ANALYSIS_STARTED_CONSUMER, throwingSupplier, Set.of(), Map.of("b", "2"));
231231
var analyzeCommand2 = new AnalyzeCommand("moduleKey", UUID.randomUUID(), TriggerType.FORCED,
232232
() -> analysisConfig, issues2::add, null, progressMonitor, TASK_MANAGER,
233-
NO_OP_ANALYSIS_STARTED_CONSUMER, ANALYSIS_READY_SUPPLIER, Set.of(), Map.of());
233+
NO_OP_ANALYSIS_STARTED_CONSUMER, ANALYSIS_READY_SUPPLIER, Set.of(), Map.of("c", "3"));
234234

235235
analysisScheduler.post(analyzeCommand1);
236236
analysisScheduler.post(throwingCommand);
@@ -242,6 +242,22 @@ def foo():
242242
assertThat(issues2).hasSize(1);
243243
}
244244

245+
@Test
246+
void should_not_queue_command_if_already_canceled(@TempDir Path baseDir) {
247+
var analysisConfig = AnalysisConfiguration.builder()
248+
.addActiveRules(trailingCommentRule())
249+
.setBaseDir(baseDir)
250+
.build();
251+
var analyzeCommand = new AnalyzeCommand("moduleKey", UUID.randomUUID(), TriggerType.FORCED,
252+
() -> analysisConfig, i -> {}, null, progressMonitor, TASK_MANAGER,
253+
NO_OP_ANALYSIS_STARTED_CONSUMER, ANALYSIS_READY_SUPPLIER, Set.of(), Map.of("a", "1"));
254+
progressMonitor.cancel();
255+
256+
analysisScheduler.post(analyzeCommand);
257+
258+
await().untilAsserted(() -> assertThat(logTester.logs()).contains("Not picking next command " + analyzeCommand + ", is canceled"));
259+
}
260+
245261
@Test
246262
void should_interrupt_executing_thread_when_stopping(@TempDir Path baseDir) throws IOException {
247263
var content = """

‎backend/commons/src/main/java/org/sonarsource/sonarlint/core/commons/progress/TaskManager.java‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,21 @@ public class TaskManager {
2828
private static final ProgressMonitor NO_OP = new NoOpProgressMonitor();
2929
private final ConcurrentHashMap<String, ProgressMonitor> progressMonitorsByTaskId = new ConcurrentHashMap<>();
3030

31-
public final void runTask(@Nullable String configurationScopeId, UUID taskId, String title, @Nullable String message, boolean indeterminate, boolean cancellable, Task task,
31+
public final void createAndRunTask(@Nullable String configurationScopeId, UUID taskId, String title, @Nullable String message,
32+
boolean indeterminate, boolean cancellable, Task task,
3233
SonarLintCancelMonitor cancelMonitor) {
33-
var progressMonitor = startProgress(configurationScopeId, taskId, title, message, indeterminate, cancellable, cancelMonitor);
34-
progressMonitorsByTaskId.put(taskId.toString(), progressMonitor);
34+
trackNewTask(taskId, cancelMonitor);
35+
runExistingTask(configurationScopeId, taskId, title, message, indeterminate, cancellable, task, cancelMonitor);
36+
}
37+
38+
public final void runExistingTask(@Nullable String configurationScopeId, UUID taskId, String title, @Nullable String message,
39+
boolean indeterminate, boolean cancellable, Task task, SonarLintCancelMonitor cancelMonitor) {
40+
var progressMonitor = progressMonitorsByTaskId.get(taskId.toString());
41+
if (progressMonitor == null) {
42+
SonarLintLogger.get().debug("Cannot run unknown task '{}'", taskId);
43+
return;
44+
}
45+
startProgress(configurationScopeId, taskId, title, message, indeterminate, cancellable, cancelMonitor);
3546
try {
3647
task.run(progressMonitor);
3748
} finally {
@@ -40,6 +51,11 @@ public final void runTask(@Nullable String configurationScopeId, UUID taskId, St
4051
}
4152
}
4253

54+
public final void trackNewTask(UUID taskId, SonarLintCancelMonitor cancelMonitor) {
55+
var progressMonitor = createProgress(taskId, cancelMonitor);
56+
progressMonitorsByTaskId.put(taskId.toString(), progressMonitor);
57+
}
58+
4359
public void cancel(String taskId) {
4460
SonarLintLogger.get().debug("Cancelling task from RPC request {}", taskId);
4561
var progressMonitor = progressMonitorsByTaskId.remove(taskId);
@@ -48,9 +64,13 @@ public void cancel(String taskId) {
4864
}
4965
}
5066

51-
protected ProgressMonitor startProgress(@Nullable String configurationScopeId, UUID taskId, String title, @Nullable String message, boolean indeterminate, boolean cancellable,
67+
protected void startProgress(@Nullable String configurationScopeId, UUID taskId, String title, @Nullable String message, boolean indeterminate, boolean cancellable,
5268
SonarLintCancelMonitor cancelMonitor) {
5369
// can be overridden
70+
}
71+
72+
protected ProgressMonitor createProgress(UUID taskId, SonarLintCancelMonitor cancelMonitor) {
73+
// can be overridden
5474
return NO_OP;
5575
}
5676
}

0 commit comments

Comments
 (0)