Protect caches from being modified concurrently #746
Merged
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.
Fixes this crash.
The only way I can see the crash occurring is if two evaluations for the same thing happen at once. In that case, we could end up modifying the index for the same key concurrently, creating the scenario which could cause a crash (an
null
returned for a key we just checked).My solution here is to
synchronize
accesses to the index (and doing the same thing for our comparison cache for good measure) to prevent them happening concurrently.What has been done to verify that this works as intended?
It's really hard to test this stuff without injecting some kind of executor into the objects and this can often result in pretty contrived examples. The best I can do here is to reason about the possible concurrent accesses.
Why is this the best possible solution? Were any other approaches considered?
An alternative to using
synchronized
here would be to use a lock in theFilterStrategy
that can be conditionally acquired. If the lock can't be acquired, we could actually just fall through to the next strategy hoping that that is thread safe (or not currently locked by another thread at least). We'd probably want bothEqualityExpressionIndexFilterStrategy
andComparisonExpressionCacheFilterStrategy
to behave like this. I'm assuming thatRawFilterStrategy
is thread safe. I think I like that more than the PR solution (and it's also far easier to test), but the PR solution is far simpler and swifter fix for the crash as far as I can tell.Falling through on a failed lock attempt would be a good follow up.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
The risk here is a performance drop if a thread ends up waiting for a lock. This is especially bad news in Collect if the thread waiting for the lock is the UI/main one. It's hard to say whether that scenario is better or worse than the thread actually taking the time to evaluate the filter however.