Fix deadlock between Cache.put and invalidateAll #99480
Conversation
elasticsearchmachine
added
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @thecoop, I've created a changelog YAML for you. |
|
This is probably worth backporting, maybe even to 7.17 |
|
The test added here doesn't actually find the original bug. I'm not sure how to even create a test for this case - in which case I might as well just remove it... |
thecoop
added
v8.10.0
v7.17.14
v8.10.1
auto-backport-and-merge
and removed
v8.10.0
labels
|
I think it is extremely hard to find. I tried using |
|
I've removed the test, given the problem was quite a basic one (incorrect lock acquisition order) and the fix is clear |
|
@elasticmachine update branch |
|
@elasticsearchmachine rerun elasticsearch-ci/part-1 |
There was a problem hiding this comment.
This still looks fine, but even if the situation is rare, ensuring the order of releasing locks seems a worthwhile thing to test. I don't feel that strongly about it (merge if you like), but I have the following suggestion for how to test:
The CacheSegment is what holds the lock objects which we would want to mock. I think a package private ctor for Cache could take the ctor for CacheSegment. Then move the construction of the read/write lock to an overridable method, remove the final (no reason for it to be final really anyways since it is private). Have a TestCacheSegment which subclasses and creates a delegate, so that you can hook into when locking/unlocking happens, and then assert on the order. Again, this is just an idea, I realize it is a bit of a change, but IMO not too much to ensure something that can result in a deadlock.
The invalidateAll method is taking out the lru lock and segment locks in a different order to the put method, when the put is replacing an existing value. This results in a deadlock between the two methods as they try to swap locks. This fixes it by making sure invalidateAll takes out locks in the same order as put. This is difficult to test because the put needs to be replacing an existing value, and invalidateAll clears the cache, resulting in subsequent puts not hitting the deadlock condition. A test that overrides some internal implementations to expose this particular deadlock will be coming later.
The invalidateAll method is taking out the lru lock and segment locks in a different order to the put method, when the put is replacing an existing value. This results in a deadlock between the two methods as they try to swap locks. This fixes it by making sure invalidateAll takes out locks in the same order as put. This is difficult to test because the put needs to be replacing an existing value, and invalidateAll clears the cache, resulting in subsequent puts not hitting the deadlock condition. A test that overrides some internal implementations to expose this particular deadlock will be coming later.
The invalidateAll method is taking out the lru lock and segment locks in a different order to the put method, when the put is replacing an existing value. This results in a deadlock between the two methods as they try to swap locks. This fixes it by making sure invalidateAll takes out locks in the same order as put. This is difficult to test because the put needs to be replacing an existing value, and invalidateAll clears the cache, resulting in subsequent puts not hitting the deadlock condition. A test that overrides some internal implementations to expose this particular deadlock will be coming later.
This fixes #99326
Unfortunately testing this is exceptionally difficult - hitting the second lock in
putrequires the item to be there already, butinvalidateAllremoves all items from the cache, meaning the put doesn't hit the second lock. The test I've added doesn't trigger the deadlock condition on the old code after 2000 runs