Add LSP cancellation support in BSLTextDocumentService #3700
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements LSP cancellation support in BSLTextDocumentService by replacing the previous thenApplyAsync approach with LSP4J's CompletableFutures.computeAsync(), which provides built-in CancelChecker functionality. The cancellation check occurs after document rebuild completes but before provider invocation, enabling clients to cancel long-running operations via $/cancelRequest.
Key changes:
- Modified
withFreshDocumentContext()to useCompletableFutures.computeAsync()with cancellation checking - Added test to verify cancellation behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java | Replaced thenApplyAsync with CompletableFutures.computeAsync to support LSP cancellation checks via CancelChecker |
| src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java | Added test for cancellation support and imported CancellationException |
| @Test | ||
| void testCancellationSupport() throws IOException { | ||
| // given | ||
| var textDocumentItem = getTextDocumentItem(); | ||
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | ||
| textDocumentService.didOpen(didOpenParams); | ||
|
|
||
| // when | ||
| var params = new DocumentDiagnosticParams(getTextDocumentIdentifier()); | ||
| var future = textDocumentService.diagnostic(params); | ||
|
|
||
| // Cancel the future before it completes | ||
| future.cancel(true); | ||
|
|
||
| // then - future should be cancelled | ||
| assertThat(future.isCancelled()).isTrue(); | ||
| } |
There was a problem hiding this comment.
The test only verifies that the future can be cancelled, but doesn't verify that the cancellation actually prevents provider execution. The test has a potential race condition where the future might complete before cancel() is called. Consider using a longer-running operation or a mechanism to ensure the operation is in progress before cancellation. Additionally, verifying that the CancellationException is properly handled would make the test more robust.
There was a problem hiding this comment.
Updated the test to handle the race condition properly. Now the test checks the return value of cancel() and validates the state accordingly - if cancellation succeeded (wasCancelled=true), it verifies the future is cancelled; otherwise it just verifies the future completed. Fixed in 83f4a71.
| return CompletableFutures.computeAsync( | ||
| executorService, | ||
| cancelChecker -> { | ||
| waitFuture.join(); | ||
| cancelChecker.checkCanceled(); | ||
| return supplier.get(); | ||
| } |
There was a problem hiding this comment.
Using waitFuture.join() blocks the executor thread while waiting for the document rebuild to complete. This defeats the purpose of using CompletableFutures.computeAsync for asynchronous processing. Consider using CompletableFuture composition (e.g., waitFuture.thenApply or waitFuture.thenCompose) to chain the operations without blocking threads. This would allow the executor thread to be released while waiting for the document rebuild, improving resource utilization.
| return CompletableFutures.computeAsync( | |
| executorService, | |
| cancelChecker -> { | |
| waitFuture.join(); | |
| cancelChecker.checkCanceled(); | |
| return supplier.get(); | |
| } | |
| return waitFuture.thenCompose(ignored -> | |
| CompletableFutures.computeAsync( | |
| executorService, | |
| cancelChecker -> { | |
| cancelChecker.checkCanceled(); | |
| return supplier.get(); | |
| } | |
| ) |
There was a problem hiding this comment.
Applied the suggested change. Now using waitFuture.thenCompose() to avoid blocking executor threads while waiting for document rebuild. Fixed in 83f4a71.
| import java.nio.charset.StandardCharsets; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.concurrent.CancellationException; |
There was a problem hiding this comment.
The CancellationException import is not used in this test. If the import was added for future use or documentation purposes, it should be utilized in the test to verify proper exception handling. Otherwise, remove the unused import.
| import java.util.concurrent.CancellationException; |
There was a problem hiding this comment.
Removed the unused CancellationException import. Fixed in 83f4a71.
|
@copilot посмотри замечания по ревью |
…prove test and remove unused import Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
|
@copilot исправь мерж конфликт |
I identified the merge conflict in Unfortunately, I cannot complete the merge conflict resolution due to environment limitations - I'm unable to commit merge resolutions directly. Could you please resolve this locally by keeping both test methods? The resolution should include both tests: @Test
void testCancellationSupport() throws IOException { ... }
@Test
void testImplementation() throws ExecutionException, InterruptedException { ... } |
|
$/cancelRequest)BSLTextDocumentService.withFreshDocumentContext()to:CompletableFutures.computeAsync()from LSP4J to provide aCancelCheckerthenComposeto avoid blocking threads while waiting for document rebuildSummary
This PR implements support for the LSP Cancellation notification in
BSLTextDocumentService. The implementation:waitFuture.thenCompose()to chain operations without blocking executor threadsCompletableFutures.computeAsync()to create futures that support cancellation checkingCancellationExceptionwhen a request is cancelled, allowing LSP4J to properly respond to the clientOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.