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

Show file tree
Hide file tree
Changes from all commits
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
26 changes: 26 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 @@ -140,6 +140,7 @@
public class BSLTextDocumentService implements TextDocumentService, ProtocolExtension {

private static final long AWAIT_CLOSE = 30;
private static final long AWAIT_FORCE_TERMINATION = 1;

private final ServerContext context;
private final LanguageServerConfiguration configuration;
Expand Down Expand Up @@ -530,9 +531,34 @@ 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
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) {
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.
LOGGER.warn(
"Interrupted again while waiting for document executor for URI {} to terminate after shutdownNow",
uri
);
}
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();
}
}
Expand Down
87 changes: 87 additions & 0 deletions src/test/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,93 @@ void didClose() {
textDocumentService.didClose(params);
}

@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();
}
Comment on lines +204 to +236
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 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.

Copilot uses AI. Check for mistakes.

@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();
}
Comment on lines +238 to +267
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.

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.

Copilot uses AI. Check for mistakes.

@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();
}
Comment on lines +269 to +289
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.

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).

Copilot uses AI. Check for mistakes.

@Test
void didClosePublishesEmptyDiagnosticsWhenClientDoesNotSupportPullDiagnostics() throws IOException {
// given - open a document
Expand Down
Loading