Add leak detection to Store #121482
Open
Add leak detection to Store #121482
Conversation
This file contains 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
nicktindall
added
>test
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
the
Team:Distributed Coordination
nicktindall
commented
Feb 3, 2025
| ShardLock shardLock, | ||
| OnClose onClose, | ||
| boolean hasIndexSort, | ||
| boolean detectLeaks |
There was a problem hiding this comment.
I don't like the need to be able to turn leak detection off, but there were a few places where we don't get access to the store to manage its lifecycle correctly.
nicktindall
commented
Feb 3, 2025
| Store.OnClose.EMPTY, | ||
| false, | ||
| false | ||
| ); |
There was a problem hiding this comment.
This is one such case, alternatively we could add it to a collection to be closed upon @After.
nicktindall
commented
Feb 3, 2025
| @@ -566,8 +566,7 @@ private void testDirectories( | |||
| } | |||
|
|
|||
| final Store store = new Store(shardId, indexSettings, directory, new DummyShardLock(shardId)); | |||
| store.incRef(); | |||
| releasables.add(store::decRef); | |||
| releasables.add(store::close); | |||
There was a problem hiding this comment.
I'm not sure why the lifecycle was being managed like above, and whether I've fundamentally changed something here. It seems like we should have a single reference from creation, so we don't need to incRef?
nicktindall
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When working on ES-10641, I noticed one category of slow shut down is due to the only possible relocation failing repeatedly, which is in turn caused by the the shard lock being held on the target node.
The message on the shard lock is
closing shardwhich indicates the lock was once held by aStorewhich never released the shard lock.We recently made a change to dump hot threads when shard creation was prevented by a lingering shard lock, and in instances of the above error occurring you can now see there is no active thread doing anything related to closing the shard, so I suspect there may be a store reference leaking somewhere, preventing the final release of the shard lock.
This PR adds leak tracking to the
Store's refCounter in the hope that we might observe the leak in CI.