Исправлена гонка на заполнении контекста и LSP операциях #3729
Conversation
Аспект отправки событий тригерился на оба rebuild, и внешний и внутренний.
Добавлены блокировки на уровне URI вместо общего лока на уровне сервера. Это должно решить проблему отдачи недоинициализированного контекта во время работы редактора + дать возможность параллельной работы LSP операций и заполнении контекста
|
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. 📝 WalkthroughWalkthroughThis PR introduces per-document ReadWriteLocks and context-safe execution wrappers. BSLTextDocumentService now uses per-URI locking and withFreshDocumentContext(...) helpers; DocumentChangeExecutor and DocumentContext are updated to acquire per-document locks and rebuild from filesystem respectively; ServerContext manages per-document locks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BSLTextDocumentService
participant ServerContext
participant DocumentContext
participant DocumentChangeExecutor
rect rgb(220,240,255)
Note over Client,BSLTextDocumentService: didOpen flow (write lock)
end
Client->>BSLTextDocumentService: didOpen(uri, content)
BSLTextDocumentService->>ServerContext: getDocumentLock(uri)
ServerContext-->>BSLTextDocumentService: ReadWriteLock
BSLTextDocumentService->>ServerContext: addDocument(uri)
ServerContext-->>BSLTextDocumentService: DocumentContext
BSLTextDocumentService->>DocumentChangeExecutor: create executor(ctx)
BSLTextDocumentService->>DocumentContext: open(document content)
BSLTextDocumentService->>BSLTextDocumentService: runDiagnostics() (under write lock)
BSLTextDocumentService-->>Client: open acknowledgment
rect rgb(220,240,255)
Note over Client,BSLTextDocumentService: didChange flow (read lock + executor)
end
Client->>BSLTextDocumentService: didChange(uri, changes)
BSLTextDocumentService->>ServerContext: getDocumentLock(uri)
ServerContext-->>BSLTextDocumentService: ReadWriteLock
BSLTextDocumentService->>ServerContext: getDocument(uri)
ServerContext-->>BSLTextDocumentService: DocumentContext
BSLTextDocumentService->>DocumentChangeExecutor: submitChanges(changes)
DocumentChangeExecutor->>DocumentChangeExecutor: queue changes
DocumentChangeExecutor->>DocumentChangeExecutor: flushPendingChanges() (acquire write lock)
DocumentChangeExecutor->>DocumentContext: applyChanges()
DocumentContext-->>DocumentChangeExecutor: applied
DocumentChangeExecutor-->>BSLTextDocumentService: CompletableFuture result
BSLTextDocumentService-->>Client: change acknowledgment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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/test/java/**/*Test.java📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/**/*.java📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/main/java/**/*.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). (12)
🔇 Additional comments (5)
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 |
Test Results 2 808 files ±0 2 808 suites ±0 52m 1s ⏱️ + 2m 31s Results for commit de0ee24. ± Comparison against base commit 9c1f478. This pull request removes 8 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR addresses race conditions in document handling by introducing per-document locking mechanisms. The changes replace a global context lock with fine-grained ReadWriteLock instances for each document URI, improving concurrency and preventing race conditions during parallel document operations.
Key Changes
- Introduced per-document
ReadWriteLockinstances stored in aConcurrentHashMapto replace the global context lock - Updated document lifecycle operations (open, change, close) to acquire appropriate locks before modifying document state
- Added lock acquisition in
DocumentChangeExecutor.flushPendingChanges()to protect document rebuild operations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ServerContext.java | Replaced global contextLock with per-document documentLocks map; refactored populateContext, getDocument, and addDocument to use document-specific locks; added getDocumentLock() method |
| DocumentContext.java | Renamed rebuild() to rebuildFromFileSystem() for clarity |
| DocumentChangeExecutor.java | Added write lock acquisition in flushPendingChanges() before applying document changes |
| BSLTextDocumentService.java | Added lock acquisition in didOpen() and didChange(); refactored withFreshDocumentContext to acquire read locks; split into nullable and non-nullable variants |
| DocumentChangeExecutorTest.java | Updated test setup to mock ServerContext and provide ReentrantReadWriteLock instances; added executor cleanup before creating new instances |
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
Outdated
Show resolved
Hide resolved
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java (1)
326-333: Silent failure on IOException may leave document in inconsistent state.If
FileUtils.readFileToStringthrows anIOException, the error is logged butrebuildis never called. This could leave the document with stale or null content, potentially causingNullPointerExceptionwhengetContent()is later called (since it usesrequireNonNull(content)).Consider either propagating the exception or setting the document to a known empty/error state.
🔎 Suggested improvement
protected void rebuildFromFileSystem() { try { var newContent = FileUtils.readFileToString(new File(uri), StandardCharsets.UTF_8); rebuild(newContent, 0); } catch (IOException e) { LOGGER.error("Can't rebuild content from uri", e); + rebuild("", 0); // Initialize with empty content to avoid NPE } }src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (1)
236-246: Potential race condition when removing document lock.Removing the lock from
documentLocks(line 245) while another thread might be waiting to acquire it could lead to inconsistent behavior. A subsequent call togetDocumentLock(uri)would return a new lock instance, breaking mutual exclusion guarantees.Consider acquiring the write lock before removal to ensure no concurrent operations are in progress:
🔎 Suggested fix
public void removeDocument(URI uri) { + var lock = getDocumentLock(uri); + lock.writeLock().lock(); + try { var documentContext = documents.get(uri); if (openedDocuments.contains(documentContext)) { throw new IllegalStateException(String.format("Document %s is opened", uri)); } removeDocumentMdoRefByUri(uri); states.remove(documentContext); documents.remove(uri); documentLocks.remove(uri); + } finally { + lock.writeLock().unlock(); + } }
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (1)
215-227: Nested lock acquisition inaddDocument- verify reentrancy behavior.
addDocumentacquires a write lock (line 217), then callsgetDocument(uri)(line 219) which also acquires a read lock on the same URI (lines 167-168). WhileReentrantReadWriteLockallows a write lock holder to acquire a read lock (downgrade), this nested acquisition pattern adds unnecessary overhead.Consider accessing the
documentsmap directly inside the write lock scope instead of callinggetDocument.🔎 Suggested simplification
public DocumentContext addDocument(URI uri) { var lock = getDocumentLock(uri); lock.writeLock().lock(); try { - var documentContext = getDocument(uri); + var documentContext = documents.get(uri); if (documentContext == null) { documentContext = createDocumentContext(uri); } return documentContext; } finally { lock.writeLock().unlock(); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/**/*Test.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain or improve test coverage and use appropriate test frameworks (JUnit, AssertJ, Mockito)
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java
src/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Java 17 for language and follow Java naming conventions
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
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/context/DocumentChangeExecutor.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
🧠 Learnings (2)
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
📚 Learning: 2025-11-19T09:02:03.154Z
Learnt from: theshadowco
Repo: 1c-syntax/bsl-language-server PR: 3610
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/folding/QueryPackageFoldingRangeSupplier.java:26-26
Timestamp: 2025-11-19T09:02:03.154Z
Learning: В проекте bsl-language-server используется кастомная версия ANTLR4 от 1c-syntax (io.github.1c-syntax:antlr4), которая включает базовый класс org.antlr.v4.runtime.Tokenizer с методами getAst() и getTokens(). SDBLTokenizer и BSLTokenizer наследуются от этого базового класса.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
⏰ 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)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (java)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: Benchmark
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
🔇 Additional comments (9)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutor.java (1)
244-257: LGTM! Per-document write lock correctly protects change application.The lock acquisition pattern is correct: lock is acquired before the try block, and unlock is in the finally block. This ensures the lock is released even if
changeListener.onChangethrows.src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java (2)
52-69: LGTM! Test setup correctly mocks per-document locking dependencies.The mock wiring properly chains
documentContext.getServerContext().getDocumentLock(uri)to return aReentrantReadWriteLock, accurately reflecting the new locking architecture.
86-103: LGTM! Proper executor lifecycle management in test.The pre-cleanup pattern correctly shuts down the executor created in
setUpbefore creating a new one with a custom listener. This prevents resource leaks and potential interference between executor instances.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (2)
121-139: LGTM! Per-document locking in parallel context population.The write lock correctly protects document creation and rebuilding. Progress reporting (
tick()) is appropriately placed outside the lock scope to avoid holding locks during potentially slow operations.
258-268: LGTM! Lazy lock creation withcomputeIfAbsent.The pattern is thread-safe and efficient. Using the default unfair lock provides better throughput for typical LSP workloads.
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java (4)
469-496: LGTM!didOpencorrectly uses write lock for document initialization.The write lock appropriately protects the critical section where the document is added, executor is created, and document is opened. The conditional diagnostic validation is also correctly placed within the lock scope to prevent race conditions during initial document setup.
500-533: LGTM!didChangeuses read lock appropriately for executor access.The read lock correctly protects access to the executor map without blocking other readers. The actual document mutation is handled asynchronously by
DocumentChangeExecutorwith its own write lock, ensuring proper synchronization.
831-858: LGTM! Read-after-write consistency withawaitLatestand read lock.The pattern correctly ensures:
- All pending changes are applied via
awaitLatest()- Read lock prevents concurrent modifications during supplier execution
- Lock is released in finally block
This provides strong consistency guarantees for LSP request handlers.
817-829: LGTM! Clean wrapper pattern for nullable vs non-nullable returns.The separation between
withFreshDocumentContextNullableandwithFreshDocumentContextprovides proper type safety while sharing the same implementation.
nixel2007
changed the title
|
@copilot отработай замечания по ревью. |
|
@nixel2007 I've opened a new pull request, #3731, to work on those changes. Once the pull request is ready, I'll request review from you. |
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
Show resolved
Hide resolved
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java
Outdated
Show resolved
Hide resolved
…and nested locking in ServerContext Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
Устранена избыточная вложенная блокировка в методах работы с документами
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (1)
236-246: Add lock acquisition toremoveDocumentto prevent race conditions.The method removes documents without acquiring a write lock, violating the synchronization pattern established by
getDocument(read lock) andaddDocument(write lock). A thread callinggetDocumentcould be reading aDocumentContextwhile another thread simultaneously removes it viaremoveDocument, causing a race condition. Acquirelock.writeLock()before modifying the documents map, consistent withaddDocument:public void removeDocument(URI uri) { var lock = getDocumentLock(uri); lock.writeLock().lock(); try { var documentContext = documents.get(uri); if (openedDocuments.contains(documentContext)) { throw new IllegalStateException(String.format("Document %s is opened", uri)); } removeDocumentMdoRefByUri(uri); states.remove(documentContext); documents.remove(uri); documentLocks.remove(uri); } finally { lock.writeLock().unlock(); } }
♻️ Duplicate comments (4)
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java (1)
50-69: LGTM! Past review concern addressed.The mock setup now correctly returns the same lock instance for the same URI using
computeIfAbsent, addressing the previous review comment about lock instance consistency.src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (3)
121-139: LGTM! Past review concern addressed.The per-URI write lock is correctly acquired and released. Using
documents.get(uri)directly avoids the nested locking issue flagged in previous reviews.
216-226: LGTM! Past review concern addressed.The per-URI write lock is correctly acquired and released. Using
documents.get(uri)directly avoids the nested locking issue flagged in previous reviews.
258-268: Potential memory leak from unused locks.The
getDocumentLock()method creates locks on-demand viacomputeIfAbsent. If called for URIs that never get documents added (e.g., from external code checking documents that don't exist), locks will accumulate indocumentLockswithout cleanup. Consider:
- Adding validation to ensure the URI corresponds to an existing or soon-to-be-added document, or
- Implementing periodic cleanup of locks for non-existent documents, or
- Documenting that callers must only request locks for documents they intend to create/access
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/**/*Test.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain or improve test coverage and use appropriate test frameworks (JUnit, AssertJ, Mockito)
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java
src/**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Java 17 for language and follow Java naming conventions
Files:
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
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/context/ServerContext.java
🧠 Learnings (1)
📚 Learning: 2025-02-10T17:12:56.150Z
Learnt from: nixel2007
Repo: 1c-syntax/bsl-language-server PR: 3408
File: src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/ProtectedModuleDiagnostic.java:63-66
Timestamp: 2025-02-10T17:12:56.150Z
Learning: In BSL Language Server, `documentContext.getServerContext().getConfiguration()` is guaranteed to return a non-null value, making null checks unnecessary.
Applied to files:
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
⏰ 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). (12)
- GitHub Check: Benchmark
- GitHub Check: build
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: Analyse
🔇 Additional comments (5)
src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentChangeExecutorTest.java (2)
29-47: LGTM!The imports and constant declaration properly support the per-URI locking test infrastructure.
97-99: LGTM!The pre-cleanup properly shuts down the executor from setUp before creating a new one, preventing resource leaks and test interference.
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (3)
87-87: LGTM!The per-URI lock map using ConcurrentHashMap is appropriate for thread-safe management of document locks.
167-174: LGTM!The read lock acquisition properly protects document access in the new per-URI locking model.
312-319: LGTM!The method call correctly uses
rebuildFromFileSystem()to reflect the filesystem-based rebuild operation.
|
Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Для диагностик
Дополнительно
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.