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

Adding lock value as parameter to lock() #49

Closed
wants to merge 1 commit into from

Conversation

lilacdev
Copy link

Here is my idea. Not the best JS developper, perhaps this is dirty ;).

Here is my idea. Not the best JS developper, perhaps this is dirty ;).
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 99.758% when pulling 88f532c on netwoof:patch-2 into feab262 on mike-marcacci:master.

@mike-marcacci
Copy link
Owner

mike-marcacci commented Feb 24, 2019

Hi @lilacdev, thanks for the PR! While there isn't technically a problem here, I'm a bit hesitant to make this change, since it opens up a very serious footgun: passing an insufficiently random valueOrCallback would cause some serious problems in real world uses, since multiple locks would have the same value. This could allow a partially acquired lock to invalidate a different, fully-acquired lock.

It would be safe if you were to, for example, use a nonrandom prefix to the value like `foo${Redlock.prototype._random()}`, but using "foo" by itself would have potentially very bad consequences, under very unpredictable conditions.

I'll continue to think about this, but in the meantime you can simply use the "private" method, _lock(resource, value, ttl, callback) with your prefixed random value.

@mike-marcacci
Copy link
Owner

I'm going to go ahead and close this, because I don't want to encourage dangerous patterns here (per my comment above). Let me know if you have a compelling use case that truly requires this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants