Fix cleanup multiple parallel runs issue #2342
Merged
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
* The previous name was giving the wrong impression that the clean-up would happen every time the method is called. Which is not true.
* With the previous design, it was possible for an UAA instance to run 2 cleanups at the same time * This commit makes this now impossible, by adding a mutex to protect the critical section. Now only one cleanup can happen at a given time. * I am aware that this implementation can result in a cleanup not happening when it should have: in the case that thread 1 takes the lock and cannot run because it's not yet the time to clean, thread 2 gets locked out by the mutex while now enough time had passed for a new cleanup. I deem this case not a problem as the next call will properly call the cleanup method. The broad critical section makes the code simpler, instead of requiring potentially multiple mutexes.
* Now that the variable is protected by a Mutex, it doesn't need to be inside an `Atomic` anymore.
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/185222661 The labels on this github issue will be updated when the story is started. |
swalchemist
reviewed
May 19, 2023
| @@ -350,7 +350,7 @@ void testCountingTheExecutedSqlDeleteStatements() throws SQLException { | |||
| // When | |||
| for (int i = 0; i < 10; i++) { | |||
| try { | |||
| store.performExpirationClean(); | |||
| store.performExpirationCleanIfEnoughTimeHasElapsed(); | |||
There was a problem hiding this comment.
Are you confident that there is sufficient test coverage for the parallel use case?
Some questionable things in this test -
- Doesn't seem to call the method concurrently.
- With "atMost(1)" it may be non-deterministic. Why not expect exactly 1 call?
- Could it really run successfully for close to 5 minutes?
- Why ignore SQL exceptions?
hsinn0
requested changes
May 19, 2023
| } | ||
| cleanMutex.release(); |
There was a problem hiding this comment.
cleanMutex.release() needs to be inside finally because it won't be executed when actuallyPerformExpirationClean() throws DataAccessException, which also should be covered by a unit test case.
There was a problem hiding this comment.
Yes I was just thinking about putting in a finally block.
* the mutex release needs to be in a `finally` block not to create a dead lock in case an exception is thrown by the cleaning method
hsinn0
approved these changes
May 19, 2023
strehle
approved these changes
May 24, 2023
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.
The cleanup now cannot be run multiple times in parallel due to the new Mutex protection.