Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix possible deadlock spanning DB transaction and caching lock with user references #4944

Merged
merged 3 commits into from Jun 12, 2023

Conversation

mtneug
Copy link
Member

@mtneug mtneug commented May 4, 2023

The JpaUserReferenceProvider#updateUserReference updates an in-memory cache as part of the DB transaction context. Putting something in the cache will acquire a lock on the cache. If the JpaUserReferenceProvider#loadUser method is called after the start of the DB transaction but before putting the object in the cache, the cache loader will try to read the user reference from the DB and put it into the cache if it cannot be found (e.g. after a Opencast startup). This also requires a lock from the cache but will block in the DB since the DB transaction is already running. Since this deadlock uses in-memory Java locks, it cannot be detected by the DB and the DB transaction will not be canceled.

This moves the cache update outside of the DB transaction context. When multiple user reference updates run at the same time there is still a race to insert into DB/cache. However, in practice the user reference is probably the same except for the last login date.

Two other commits are cleaning up JpaUserReferenceProvider a bit.

Your pull request should…

mtneug added 3 commits May 4, 2023 17:32
…ser references

The JpaUserReferenceProvider#updateUserReference updates an in-memory cache as part of the DB transaction context.
Putting something in the cache will acquire a lock on the cache. If the JpaUserReferenceProvider#loadUser method is
called after the start of the DB transaction but before putting the object in the cache, the cache loader will try to
read the user reference from the DB and put it into the cache if it cannot be found (e.g. after a Opencast startup).
This also requires a lock from the cache but will block in the DB since the DB transaction is already running. Since
this deadlock uses in-memory Java locks, it cannot be detected by the DB and the DB transaction will not be canceled.

This moves the cache update outside of the DB transaction context. When multiple user reference updates run at the same
time there is still a race to insert into DB/cache. However, in practice the user reference is probably the same except
for the last login date.
@mtneug mtneug added bug security Pull requests that address a security vulnerability labels May 4, 2023
@mtneug
Copy link
Member Author

mtneug commented May 4, 2023

Additional info for the first commit: the last login date is updated by the login providers using JpaUserReferenceProvider. There is no need to set a new date again.

Copy link
Member

@gregorydlogan gregorydlogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me. Not a huge fan of leaving the race in, could we switch the cache to some kind of concurrent map?

@mtneug
Copy link
Member Author

mtneug commented May 9, 2023

This seems reasonable to me. Not a huge fan of leaving the race in, could we switch the cache to some kind of concurrent map?

The cache has an internal lock, so it should be threadsafe. The race I mention is between two calls of e.g. updateUserReference:

updateUserReference 1: tx finishes
updateUserReference 2: tx finishes
updateUserReference 2: cache update
updateUserReference 1: cache update (DB might have newer data)

But close calls to updateUserReference will likely not have updated user infos (apart from the last login date, maybe).

@gregorydlogan gregorydlogan self-assigned this Jun 12, 2023
@gregorydlogan gregorydlogan merged commit 5bb0381 into opencast:r/13.x Jun 12, 2023
4 checks passed
@mtneug mtneug deleted the fix-deadlock-user-ref branch June 13, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants