Skip to content

Исправлена потенциальная гонка при закрытии документа в BSLTextDocumentService. #3724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nixel2007 merged 7 commits into from
Jan 2, 2026
Merged

Исправлена потенциальная гонка при закрытии документа в BSLTextDocumentService. #3724

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
67104f4
Исправлена потенциальная гонка при закрытии документа в BSLTextDocume…
nixel2007 Jan 2, 2026
6b92861
Initial plan
Copilot Jan 2, 2026
e2c80ca
Add logging and extract timeout constant for document close termination
Copilot Jan 2, 2026
fb345fb
Add test coverage for document close race conditions
Copilot Jan 2, 2026
ff642f4
Fix grammar in log message: 'second' -> 'seconds'
Copilot Jan 2, 2026
f8abc95
Merge branch 'develop' into fix/possible-race-on-did-close
nixel2007 Jan 2, 2026
9f00ce2
Merge remote-tracking branch 'origin/copilot/sub-pr-3724' into fix/po…
nixel2007 Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,9 +530,18 @@ public void didClose(DidCloseTextDocumentParams params) {
// Wait for all queued changes to complete (with timeout to avoid hanging)
if (!docExecutor.awaitTermination(AWAIT_CLOSE, TimeUnit.SECONDS)) {
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);
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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
);
}

Copilot uses AI. Check for mistakes.
}
} catch (InterruptedException e) {
docExecutor.shutdownNow();
// Wait briefly for worker to finish after interrupt
try {
docExecutor.awaitTermination(1, TimeUnit.SECONDS);
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
} catch (InterruptedException ignored) {
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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
);

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
} catch (InterruptedException ignored) {
} catch (InterruptedException _ignored) {

Copilot uses AI. Check for mistakes.
// Already interrupted, just restore flag
}
Comment on lines +534 to +561
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Thread.currentThread().interrupt();
}
Comment on lines 534 to 563
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
Expand Down
Loading