Skip to content

Add logging and test coverage for document close race condition handling #3725

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 4 commits into from
Jan 2, 2026
+107 −3
Merged

Add logging and test coverage for document close race condition handling #3725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
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
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
23 changes: 20 additions & 3 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 @@ -532,15 +533,31 @@ public void didClose(DidCloseTextDocumentParams params) {
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);
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 {
docExecutor.awaitTermination(1, TimeUnit.SECONDS);
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) {
// Already interrupted, just restore flag
LOGGER.warn(
"Interrupted again while waiting for document executor for URI {} to terminate after shutdownNow",
uri
);
}
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 @@ -186,6 +186,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 +189 to +221
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 tests didCloseWithPendingChanges and didCloseDuringActiveChange may have race conditions. After submitting document changes with didChange(), the changes are processed asynchronously in the DocumentChangeExecutor's worker thread. The didClose() method should wait for these changes to complete, but the tests should verify that this waiting actually works correctly.

Consider using await() with atMost() (as done in other tests like didChangeIncremental() at line 116) to verify that the document changes were actually applied before closing. This would make the tests more robust and actually test the race condition handling rather than just testing that close completes.

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 +223 to +252
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.

Similar to didCloseWithPendingChanges, this test submits a change and immediately closes without verifying that the change processing would have completed. While the test verifies that didClose() completes successfully, it doesn't actually verify the race condition scenario where changes are actively processing.

Consider adding verification that the changes were actually applied, or add a small delay before closing to force the race condition scenario. This would more thoroughly test the synchronization logic in didClose().

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 +254 to +274
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 appears to be redundant with the existing didClose() test at line 183. Both tests simply open a document and immediately close it without any intervening changes. The test doesn't actually verify "executor termination delays" as mentioned in the PR description because there are no pending operations to delay termination.

Consider either removing this test or modifying it to actually test a scenario with delays (e.g., by mocking the DocumentChangeExecutor to simulate slow termination).

Copilot uses AI. Check for mistakes.

@Test
void didSave() {
DidSaveTextDocumentParams params = new DidSaveTextDocumentParams();
Expand Down