Исправлена потенциальная гонка при закрытии документа в BSLTextDocumentService. #3724
Conversation
|
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. 📝 WalkthroughWalkthroughdidClose in BSLTextDocumentService now waits for the document executor to terminate, and if it times out calls shutdownNow and waits an additional 1 second (AWAIT_FORCE_TERMINATION). InterruptedException handling also calls shutdownNow, waits the extra second, logs if termination fails, and preserves the thread interrupt status. Tests added to exercise close behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)src/main/java/**/*.java📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/*.java📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/test/java/**/*Test.java📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-02-10T17:12:56.150ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java (1)
531-546: Race condition fix is sound; consider these maintainability improvements.The enhanced shutdown logic correctly addresses the race condition by ensuring worker threads complete cleanup operations before document closure. The nested try-catch for interrupt handling is correct and properly restores interrupt status.
Suggested improvements:
Extract timeout constant: The 1-second timeout after
shutdownNow()appears in two places (lines 536 and 541). Extract it asprivate static final long AWAIT_SHUTDOWN_NOW = 1;alongsideAWAIT_CLOSEfor consistency and maintainability.Add logging for diagnostics: When
awaitTerminationtimes out, add warning logs to help diagnose shutdown issues in production. The class has@Slf4jannotation available.Verify assumption: The comment referencing "finally block in worker may still be executing flushPendingChanges" accurately describes
DocumentChangeExecutor.runWorker()(line 187), but consider adding a JavaDoc comment explaining the wait strategy for future maintainers.💡 Example implementation
private static final long AWAIT_CLOSE = 30; +private static final long AWAIT_SHUTDOWN_NOW = 1; if (!docExecutor.awaitTermination(AWAIT_CLOSE, TimeUnit.SECONDS)) { + log.warn("Document executor did not terminate within {} seconds, forcing shutdown: {}", AWAIT_CLOSE, uri); docExecutor.shutdownNow(); - docExecutor.awaitTermination(1, TimeUnit.SECONDS); + if (!docExecutor.awaitTermination(AWAIT_SHUTDOWN_NOW, TimeUnit.SECONDS)) { + log.warn("Document executor still running after shutdownNow: {}", uri); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/main/java/**/*.java: Follow the Style Guide for code formatting and conventions
Use Lombok annotations to reduce boilerplate code and enable annotation processing in IDE
Write JavaDoc for public APIs and include comments for complex logic
Use meaningful, descriptive names for classes and methods following Java naming conventions
Optimize imports before committing; DO NOT optimize imports across the entire project unless specifically working on that task
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
src/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Java 17 for language and follow Java naming conventions
Files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
There was a problem hiding this comment.
Pull request overview
This PR fixes a potential race condition in BSLTextDocumentService.didClose() by ensuring the worker thread completes execution (including its finally block) before proceeding with document closure. The fix addresses scenarios where shutdownNow() is called but the worker's finally block might still be executing flushPendingChanges().
Key changes:
- Added
awaitTermination(1, TimeUnit.SECONDS)aftershutdownNow()calls to wait for worker thread completion - Added nested try-catch in the InterruptedException handler to gracefully handle interruptions during the additional wait
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); | ||
| } catch (InterruptedException ignored) { |
There was a problem hiding this comment.
If the awaitTermination call at line 541 returns false (indicating the worker thread did not terminate within 1 second after interrupt), the method proceeds to restore the interrupt flag and continue. This means the document closure continues while the worker thread might still be executing, potentially accessing shared state. Consider logging this condition to help diagnose issues where worker threads fail to terminate promptly.
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); | |
| } catch (InterruptedException ignored) { | |
| boolean terminated = docExecutor.awaitTermination(1, TimeUnit.SECONDS); | |
| if (!terminated) { | |
| log.warn( | |
| "Document executor for URI {} did not terminate within 1 second after interrupt during document close", | |
| uri | |
| ); | |
| } | |
| } catch (InterruptedException ignored) { | |
| log.warn( | |
| "Interrupted again while waiting for document executor for URI {} to terminate after shutdownNow", | |
| uri | |
| ); |
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| docExecutor.shutdownNow(); | ||
| // Wait briefly for worker to finish after interrupt | ||
| try { | ||
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
The 1-second timeout for awaiting worker thread termination after shutdownNow is hardcoded in multiple locations (lines 535 and 541). Consider extracting this value to a named constant (e.g., AWAIT_FORCE_TERMINATION) to improve maintainability and make the timeout value more discoverable and adjustable.
| docExecutor.shutdownNow(); | ||
| // Must wait for worker thread to finish even after shutdownNow, | ||
| // because finally block in worker may still be executing flushPendingChanges | ||
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
If the awaitTermination call at line 535 returns false (indicating the worker thread did not terminate within 1 second), the method proceeds without any logging or additional handling. This means the document closure continues while the worker thread might still be executing flushPendingChanges in its finally block, potentially leading to the race condition this change aims to fix. Consider logging this condition or implementing additional handling to ensure safe cleanup.
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); | |
| boolean terminated = docExecutor.awaitTermination(1, TimeUnit.SECONDS); | |
| if (!terminated) { | |
| log.warn( | |
| "Document executor for URI {} did not terminate within the additional timeout after shutdownNow()", | |
| uri | |
| ); | |
| } |
| // Must wait for worker thread to finish even after shutdownNow, | ||
| // because finally block in worker may still be executing flushPendingChanges | ||
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| docExecutor.shutdownNow(); | ||
| // Wait briefly for worker to finish after interrupt | ||
| try { | ||
| docExecutor.awaitTermination(1, TimeUnit.SECONDS); | ||
| } catch (InterruptedException ignored) { | ||
| // Already interrupted, just restore flag | ||
| } | ||
| Thread.currentThread().interrupt(); | ||
| } |
There was a problem hiding this comment.
The added concurrency logic for handling race conditions during document closure lacks test coverage. The existing didClose test at line 183 in BSLTextDocumentServiceTest.java is very basic and doesn't test the scenario where pending changes are being flushed when shutdown occurs. Consider adding tests for: (1) didClose with pending changes in the executor queue, (2) didClose during an active didChange operation, (3) behavior when awaitTermination timeout expires.
|
@copilot посмотри замечания по ревью |
|
@nixel2007 I've opened a new pull request, #3725, to work on those changes. Once the pull request is ready, I'll request review from you. |
Test Results 2 808 files ±0 2 808 suites ±0 51m 50s ⏱️ - 1m 36s Results for commit f8abc95. ± Comparison against base commit a11189a. This pull request removes 8 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…ssible-race-on-did-close
| @Test | ||
| void didCloseAwaitTerminationCompletes() throws IOException { | ||
| // given - open a document | ||
| var textDocumentItem = getTextDocumentItem(); | ||
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | ||
| textDocumentService.didOpen(didOpenParams); | ||
|
|
||
| var uri = textDocumentItem.getUri(); | ||
| var documentContext = serverContext.getDocumentUnsafe(uri); | ||
| assertThat(documentContext).isNotNull(); | ||
| assertThat(serverContext.isDocumentOpened(documentContext)).isTrue(); | ||
|
|
||
| // when - close the document (which should wait for executor to terminate) | ||
| var closeParams = new DidCloseTextDocumentParams(); | ||
| closeParams.setTextDocument(new TextDocumentIdentifier(uri)); | ||
| textDocumentService.didClose(closeParams); | ||
|
|
||
| // then - verify the document is properly closed | ||
| // The close should complete successfully even if executor needs time to terminate | ||
| assertThat(serverContext.isDocumentOpened(documentContext)).isFalse(); | ||
| } |
There was a problem hiding this comment.
This test doesn't add meaningful test coverage beyond what didClose() already tests. The test opens and immediately closes a document without any changes, which doesn't exercise the race condition fix at all. The await termination logic being tested here is already covered by the normal document close flow. Consider removing this test or making it more specific by somehow verifying that the executor actually waited for termination (e.g., with mocking or timing assertions).
| // Must wait for worker thread to finish even after shutdownNow, | ||
| // because finally block in worker may still be executing flushPendingChanges | ||
| boolean terminated = docExecutor.awaitTermination(AWAIT_FORCE_TERMINATION, TimeUnit.SECONDS); | ||
| if (!terminated) { | ||
| LOGGER.warn( | ||
| "Document executor for URI {} did not terminate within the additional timeout after shutdownNow()", | ||
| uri | ||
| ); | ||
| } | ||
| } | ||
| } catch (InterruptedException e) { | ||
| docExecutor.shutdownNow(); | ||
| // Wait briefly for worker to finish after interrupt | ||
| try { | ||
| boolean terminated = docExecutor.awaitTermination(AWAIT_FORCE_TERMINATION, TimeUnit.SECONDS); | ||
| if (!terminated) { | ||
| LOGGER.warn( | ||
| "Document executor for URI {} did not terminate within {} seconds after interrupt during document close", | ||
| uri, | ||
| AWAIT_FORCE_TERMINATION | ||
| ); | ||
| } | ||
| } catch (InterruptedException ignored) { | ||
| LOGGER.warn( | ||
| "Interrupted again while waiting for document executor for URI {} to terminate after shutdownNow", | ||
| uri | ||
| ); | ||
| } |
There was a problem hiding this comment.
The logic for waiting after shutdownNow() is duplicated in two places - once inside the if block when normal termination times out (lines 534-542) and again in the catch block for InterruptedException (lines 547-561). Consider extracting this common logic into a private helper method to improve maintainability and reduce code duplication. The helper method could accept the URI and handle the awaitTermination call with appropriate logging.
| AWAIT_FORCE_TERMINATION | ||
| ); | ||
| } | ||
| } catch (InterruptedException ignored) { |
There was a problem hiding this comment.
The variable 'ignored' is declared but never used in the catch block. While this is a common pattern for documenting intentional suppression of exceptions, the variable should actually be named with an underscore prefix (e.g., '_') or simply omit the variable name entirely in modern Java, as the catch block already has explanatory logging that makes the intention clear.
| } catch (InterruptedException ignored) { | |
| } catch (InterruptedException _ignored) { |
| @Test | ||
| void didCloseWithPendingChanges() throws IOException { | ||
| // given - open a document and make changes | ||
| var textDocumentItem = getTextDocumentItem(); | ||
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | ||
| textDocumentService.didOpen(didOpenParams); | ||
|
|
||
| var documentContext = serverContext.getDocumentUnsafe(textDocumentItem.getUri()); | ||
| assertThat(documentContext).isNotNull(); | ||
| assertThat(serverContext.isDocumentOpened(documentContext)).isTrue(); | ||
|
|
||
| // when - submit multiple changes rapidly and then close immediately | ||
| var params = new DidChangeTextDocumentParams(); | ||
| var uri = textDocumentItem.getUri(); | ||
|
|
||
| for (int i = 0; i < 5; i++) { | ||
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2 + i)); | ||
| var range = Ranges.create(0, 0, 0, 0); | ||
| var changeEvent = new TextDocumentContentChangeEvent(range, "// Change " + i + "\n"); | ||
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | ||
| contentChanges.add(changeEvent); | ||
| params.setContentChanges(contentChanges); | ||
| textDocumentService.didChange(params); | ||
| } | ||
|
|
||
| // then - close should wait for pending changes to complete | ||
| var closeParams = new DidCloseTextDocumentParams(); | ||
| closeParams.setTextDocument(new TextDocumentIdentifier(uri)); | ||
| textDocumentService.didClose(closeParams); | ||
|
|
||
| // verify the document is closed | ||
| assertThat(serverContext.isDocumentOpened(documentContext)).isFalse(); | ||
| } |
There was a problem hiding this comment.
The test submits changes synchronously in a loop and then immediately closes the document, but this doesn't effectively test the race condition the PR aims to fix. Since didChange() only queues changes asynchronously and returns immediately, all the changes are likely queued before close is called. The test would benefit from verifying that changes are actually being processed (e.g., by checking the final document content contains all expected changes) to confirm that the close operation properly waited for the pending changes to complete.
| @Test | ||
| void didCloseDuringActiveChange() throws IOException { | ||
| // given - open a document | ||
| var textDocumentItem = getTextDocumentItem(); | ||
| var didOpenParams = new DidOpenTextDocumentParams(textDocumentItem); | ||
| textDocumentService.didOpen(didOpenParams); | ||
|
|
||
| var documentContext = serverContext.getDocumentUnsafe(textDocumentItem.getUri()); | ||
| assertThat(documentContext).isNotNull(); | ||
| assertThat(serverContext.isDocumentOpened(documentContext)).isTrue(); | ||
|
|
||
| // when - submit a change | ||
| var params = new DidChangeTextDocumentParams(); | ||
| var uri = textDocumentItem.getUri(); | ||
| params.setTextDocument(new VersionedTextDocumentIdentifier(uri, 2)); | ||
| var range = Ranges.create(0, 0, 0, 0); | ||
| var changeEvent = new TextDocumentContentChangeEvent(range, "// New content\n"); | ||
| List<TextDocumentContentChangeEvent> contentChanges = new ArrayList<>(); | ||
| contentChanges.add(changeEvent); | ||
| params.setContentChanges(contentChanges); | ||
| textDocumentService.didChange(params); | ||
|
|
||
| // then - close immediately while change may still be processing | ||
| var closeParams = new DidCloseTextDocumentParams(); | ||
| closeParams.setTextDocument(new TextDocumentIdentifier(uri)); | ||
| textDocumentService.didClose(closeParams); | ||
|
|
||
| // verify the document is closed | ||
| assertThat(serverContext.isDocumentOpened(documentContext)).isFalse(); | ||
| } |
There was a problem hiding this comment.
This test is essentially the same as didCloseWithPendingChanges but with only a single change. It doesn't add much value since it tests the same code path. Consider either removing this test or making it distinct by introducing a more realistic scenario - for example, using Thread.sleep() or a CountDownLatch to ensure the close is called while the change is actually being processed in the executor thread, rather than just queued.
|
Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.