Re-raise Redis connection errors in #lock
#110
Conversation
|
It would be great to get this in if possible, this has bitten us twice now in production resulting in data fixes 😅 If there are worries about breaking changes in the API in a way that makes this unreleasable would it be possible to add some form of either callback or logging so we can see when a lock has failed to acquire because of a redis connection error vs it actually being locked? |
|
PR looks good, change looks sensible to me. With regards to the API changes: Sure, technically incompatible, albeit maybe a minor case since it's on the error path anyway? However, if you want to bump the major version, sure, that should alert more people to the CHANGELOG. ...which doesn't exist yet, maybe a good time to create one now? @leandromoreira Sorry for not responding to your ping for so long, I'm currently on an extended time-off. Besides, I haven't been using Ruby or Redlock myself for the past years, unfortunately, so am generally not paying much attention anymore. Hence, I guess it's best if I officially stepped down as your co-host of this fabulous show 🎪 Sorry to let you hanging like this. |
|
Thanks for the update @maltoe, hope you are doing well on your time off. Seems like we'll want to merge this and cut a new release (at least minor) and add a changelog. I looked, and think that the process for releasing a new version is not documented for this project. At this point I would recommend merging this since merging would be useful for @LyleDavis and us (could point our What do you think @leandromoreira? |
|
Any news on this? It has bitten us in production again and there isn't a neat way of reaching into the gem to change the behaviour without monkey-patching at the moment 😢 |
|
SO SORRY @LyleDavis |
No need to apologize :) you've always helped a lot, thank you :) |
|
@panozzaj @LyleDavis I'm off from my home/computer so unless @maltoe does a new release I only will be able to do it next Saturday. Sorry to let you waiting and thanks for the all the efforts. |
|
@panozzaj great work on finding this and providing PR to fix it 👍 It hit us in production in one tricky case and investigations bring us here @leandromoreira Any updates on release new version of gem? |
|
@dem sorry I'll try to do it by tomorrow |
|
It's done, thank you all people, sorry for making you wait! https://github.com/leandromoreira/redlock-rb/releases/tag/1.3.1 |
This is a fix for the example I presented in a prior issue.
Currently,
#lockreturnsfalsewhen a lock cannot be acquired, whether due to a lock already being present on the key, or a connection issue. This PR stops swallowing Redis connection errors, since they are different from the key already being locked. The Redlock user can determine how to correctly handle these errors.I updated the existing tests for the new logic. I didn't see anything in the documentation referencing the existing behavior, so didn't make any documentation changes.
Please note that this change may violate expectations for others using the library if they rely on the previous "return false on connection error" logic. I could imagine that someone would not be wrapping their logic in a
rescue, so it would fail in a previously unexpected manner. I think the maintainers may want to consider whether this is a breaking change that would need a bigger semver change.Also, thanks for maintaining this project! It has been helpful in preventing the race condition that I outlined in the issue (at least a handful of times in the last week.)