search

bsm / redislock

Simplified distributed locking implementation using Redis
Other
1.45k stars 157 forks source link

Fix data race when using RetryStrategy #31

Closed calvinxiao closed 3 years ago

calvinxiao commented 3 years ago

When using linear and exponential backoff retry strategy, there is data race when increasing the counter cnt.

calvinxiao commented 3 years ago

@dim this data race is not critical but it's breaking CI with go test -race, please take a look.

dim commented 3 years ago

Thanks for this, much appreciated, a few comments ^^

calvinxiao commented 3 years ago

I think redislock.Options should be created every time when using Obtain function, I found in our codes that redislock.Options is a global variable, which is wrong.

calvinxiao commented 3 years ago

@dim , the root problem is when redislock.Options being used as a global variable, I think it's better if we can have a Clone method for RetryStrategy, this makes sure that cnt is always 0 when Obtain executes. I think this will be better than what this PR does.

Or we can update the doc and example to show how options should be used.

What do you think?

calvinxiao commented 3 years ago

I prefer the solution in #32, then we don't need all the atomic workaround.

dim commented 3 years ago

I prefer the solution in #32, then we don't need all the atomic workaround.

As said before, I prefer not to break the API. I think using atomics is fine.