spring-data-redis icon indicating copy to clipboard operation
spring-data-redis copied to clipboard

DATAREDIS-1117 - Improve doLock method to atomic.

Open joongsoo opened this issue 5 years ago • 3 comments

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.

  • [x] You have read the Spring Data contribution guidelines.

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

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

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

  • [x] 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).

joongsoo avatar Mar 21 '20 11:03 joongsoo

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

joongsoo avatar Jul 25 '20 07:07 joongsoo

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.

mp911de avatar Oct 07 '20 13:10 mp911de

@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

joongsoo avatar Oct 24 '20 17:10 joongsoo