Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

DATAREDIS-1117 - Improve doLock method to atomic. #518

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

Open
joongsoo wants to merge 4 commits into spring-projects:2.7.x
base: 2.7.x
Choose a base branch
Loading
from joongsoo:issue/DATAREDIS-1117

Conversation

@joongsoo
Copy link

@joongsoo joongsoo commented Mar 21, 2020

Related tickets: DATAREDIS-1117.

  • doLock method is not guarantee to acquire lock, so improved it.

  • Improved doLock method is guarantee to acquire lock, so wasLocked variable is unnecessary. I removed it.

  • You have read the Spring Data contribution guidelines.

  • There is a ticket in the bug tracker for the project in our JIRA.

  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.

  • You submit test cases (unit or integration tests) that back your changes.

  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Copy link
Author

@christophstrobl Hi! I need feedback. is this pr has problem?

Copy link
Member

mp911de commented Oct 7, 2020

Thanks for your pull request. There's indeed a loophole where checkAndPotentiallyWaitUntilUnlocked waits for unlocking but the lock acquisition happens later and potentially when someone has acquired the lock.

With locking, it makes sense to reduce the handling into a single place. Instead of modifying doLock, it makes sense to put the concept of locking directly into the execute method by passing a boolean flag, whether the execute callback requires a lock so execute and checkAndPotentiallyWaitUntilUnlocked can evaluate that flag and handle locking appropriately.

joongsoo reacted with thumbs up emoji joongsoo reacted with eyes emoji

Copy link
Author

@mp911de Hi. I refactored the code based on your review. Thank you.

  • All actions need locking for atomic processing. So handled lock in the execute method. (If "put" and "putIfAbsent" are called at the same time, can broken the atomic. because put is not guarantee applied the lock. I attached scenario image)
  • Because the code for judging locks has been separated, each method can only have its own responsibility.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status: waiting-for-triage An issue we've not yet triaged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /