Hold engine read lock during reader refresh #125856
Closed
Conversation
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
tlrx
added
>bug
:Distributed Indexing/Engine
elasticsearchmachine
added
the
Team:Distributed Indexing
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
Hi @tlrx, I've created a changelog YAML for you. |
fcofdez
approved these changes
Mar 28, 2025
| } | ||
| } | ||
|
|
||
| // Some engine implementations use a references counting mechanism to avoid closing the engine until all operations |
| engineLock.readLock().lock(); | ||
| var release = true; | ||
| Engine previousEngine = null; | ||
| engineLock.writeLock().lock(); |
There was a problem hiding this comment.
Really nice simplification out of this work 🎉
| postResetNewEngineConsumer.accept(newEngine); | ||
| onNewEngine(newEngine); | ||
| engineLock.writeLock().unlock(); | ||
| // Some engine implementations use a references counting mechanism to avoid closing the engine until all operations |
There was a problem hiding this comment.
Thanks for adding the comment 👍
| closeShards(readonlyShard); | ||
| } | ||
|
|
||
| @AwaitsFix(bugUrl = "Adjust this test") |
There was a problem hiding this comment.
This should be tackled I think?
fcofdez
reviewed
Mar 28, 2025
| /** | ||
| * Indicates that the {@link #close(String, boolean, Executor, ActionListener)} has been called | ||
| */ | ||
| private final AtomicBoolean isClosing = new AtomicBoolean(); |
There was a problem hiding this comment.
I wonder why we needed to introduce this new flag?
There was a problem hiding this comment.
Only to help with some Engine.Warmer logic that is executed by the refresh listener and cannot abort early if the shard is closing.
elasticsearchmachine
added
the
serverless-linked
|
Closed in favor of #126311 Thanks Francisco for the feedback! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
We still have a potential deadlock if a thread (t1) is refreshing a reader and tries to acquire (in a non-reentrant manner) the engine read lock while holding the reader refresh lock.
If another thread (t2) is waiting for the engine write lock, the attempts to acquire engine read locks are blocked (this is how reentrant read/write locks work): t1 then blocks indefinitely for the read lock without releasing the refresh lock, potentially blocking other threads that are waiting for the refresh lock to be released.
This change changes the order in which the engine read lock and refresh locks are acquired, so that it t1 is refreshing the reader and holds the refresh lock, any attempt to acquire a read lock will be reentrant and should succeed.
The compromise here is that reader refreshes can now block the reset and closing of a shard.
Relates #124635