SemanticIndex | Add deterministic IndexStoreDB cleanup on shutdown#2503
Draft
sean7218 wants to merge 3 commits into
Draft
SemanticIndex | Add deterministic IndexStoreDB cleanup on shutdown#2503sean7218 wants to merge 3 commits into
sean7218 wants to merge 3 commits into
Conversation
- Introduced UncheckedSendableOptionalIndexStoreDB to safely wrap IndexStoreDB in a thread-safe manner. - Updated underlyingIndexStoreDB to utilize ThreadSafeBox, allowing for explicit closure and deinitialization of IndexStoreDB. - Added a close() method to release the IndexStoreDB instance immediately.
…closure in BuildServerManager shutdown - Added a close() method to the MainFilesProvider protocol to release underlying resources. - Updated the shutdown method in BuildServerManager to explicitly close the index store before shutting down the build server.
sean7218
requested review from
ahoppen,
hamishknight and
rintaro
as code owners
- Updated Package.swift to include ToolchainRegistry and IndexStoreDB as dependencies. - Added a new test in TaskSchedulerTests to verify the closing behavior of IndexStoreDB, ensuring proper resource management during asynchronous operations.
Member
|
Maybe I missed something but does this contain anything that’s not also covered by #2483? I think the problem that #2455 (comment) mentioned was that if SourceKit-LSP is not idle, there may still be lingering references to the |
Author
|
You are absolutely right! 😅 I misunderstood the problem. Let me fix it on the upstream side |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors UncheckedIndex to wrap IndexStoreDB in a ThreadSafeBox, enabling explicit lifecycle management. A new close() method sets the underlying instance to nil, triggering deterministic deinitialization rather than relying on ARC-driven deallocation.
Introduces a close() method on the MainFilesProvider protocol (with a default no-op) so BuildServerManager can tear down the index store without a circular dependency on SemanticIndex.
Calls mainFilesProvider.close() as the first step of BuildServerManager.shutdown(), ensuring the IndexStoreDB is released before the build server connection is torn down.
Motivation
Previously, the IndexStoreDB instance was only released when UncheckedIndex was deallocated, which depends on ARC timing and can lead to non-deterministic resource cleanup. In test scenarios this caused race conditions where the connection could be reported as a leak. This change gives the shutdown path explicit control over when the index store is closed.
Changes
CheckedIndex.swift — UncheckedIndex.underlyingIndexStoreDB is now a computed property backed by a ThreadSafeBox. Added close() to nil out the stored instance. Accessing the index after close triggers a preconditionFailure.
MainFilesProvider.swift — Added func close() async to the protocol with a default empty implementation.
BuildServerManager.swift — shutdown() now calls await self.mainFilesProvider?.value?.close() before proceeding with build server teardown.