SLVS-2583 Implement cancellation #19
Conversation
georgii-borovinskikh-sonarsource
force-pushed
the
gb/cancellation
branch
3 times, most recently
from
f2133be to
158c1f8
Compare
georgii-borovinskikh-sonarsource
force-pushed
the
gb/cancellation
branch
from
158c1f8 to
0f53995
Compare
|
georgii-borovinskikh-sonarsource
requested a review
from damien-urruty-sonarsource
| Collection<RoslynIssue> roslynIssues = new ArrayList<>(); | ||
| try { | ||
| var response = httpClientFactory.sendRequest(fileNames, activeRules, analysisProperties, analyzerInfo); | ||
| var response = httpClientFactory.sendAnalyzeRequest(fileNames, activeRules, analysisProperties, analyzerInfo, analysisId); |
There was a problem hiding this comment.
Nitpick and not related to your changes: is the object really a factory? I would expect the factory to have methods to create things. Here it looks more like an httpClient
There was a problem hiding this comment.
yeah the design around this area is not great, but I didn't focus on improving it much
|
|
||
| public CompletableFuture<HttpResponse<Void>> sendCancelRequest(UUID analysisId){ | ||
| var payload = jsonRequestBuilder.buildCancelBody(analysisId); | ||
| var request = createRequest(payload, "cancel"); |
There was a problem hiding this comment.
I was a bit surprised to find a separate request for the cancelation. I thought you would have used HttpClient.sendAsync, and for canceling the request you could have canceled the future returned. I am not saying it's wrong, maybe it's even better to handle cancelation on the VS side because maybe you cannot know that a request was canceled?
There was a problem hiding this comment.
There is no way to cancel a http request without breaking the connection, as it is not supported on the protocol level. Breaking the connection would defeat the purpose of reusing the client to utilize keep-alive
| private final ScheduledExecutorService scheduledExecutorService; | ||
|
|
||
| public AnalysisCancellationService() { | ||
| scheduledExecutorService = Executors.newScheduledThreadPool(5); |
There was a problem hiding this comment.
There is normally a single analysis going on at the same time, this is ensured by the scheduler in SLCORE. So you could use newSingleThreadScheduledExecutor
There was a problem hiding this comment.
Many analysis can be scheduled and cancelled in a very short time, so just to be sure we can cancel them all asap I added a small pool here. I think it's fine to keep it like this, considering this is a long-lived pool
|
|
||
| assertThat(httpClient1).isNotNull().isNotSameAs(httpClient2); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing empty line at the end of file, it's more of a convention
| LOG.error("Failed to cancel analysis due to: {}", e.getMessage(), e); | ||
| return null; | ||
| }).thenApply(response -> { | ||
| if (response.statusCode() != HttpURLConnection.HTTP_OK) { |
There was a problem hiding this comment.
You will get a NullPointerException here if the flow went in the exceptionally above, because the null that is returned will be passed as response to the current future. There is maybe a missing test there
|
|
||
| @Override | ||
| public synchronized boolean cancelIfNeeded() { | ||
| if (isCompleted){ |
There was a problem hiding this comment.
Small formatting hiccup, missing space before the opening curly brace
|
|
||
| /** | ||
| * Factory for creating AnalysisManager instances. | ||
| * This class has instance-based scope in SonarLint. |
There was a problem hiding this comment.
It doesn't, is the comment wrong or the code?
EDIT: the fact that you need a sensorContext as a constructor parameter can't work as a instance level component.
There was a problem hiding this comment.
Outdated comment, I will remove it
| * Factory for creating AnalysisManager instances. | ||
| * This class has instance-based scope in SonarLint. | ||
| */ | ||
| @ScannerSide |
There was a problem hiding this comment.
Not useful
| */ | ||
| @ScannerSide | ||
| @SonarLintSide | ||
| public class RemoteAnalysisService { |
There was a problem hiding this comment.
Maybe related to the previous comment, but for now I don't see this class as very useful. What is the original intent? Couldn't you merge it with HttpAnalysisRequestHandler?
There was a problem hiding this comment.
Just an abstraction. Makes testing a bit easier
|
|
||
| import java.net.http.HttpClient; | ||
|
|
||
| @ScannerSide |
There was a problem hiding this comment.
Not needed
SLVS-2583