-
Notifications
You must be signed in to change notification settings - Fork 83
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
Lock: Add missing null check #66
Conversation
This resulted in a NullReferenceException hiding the expected LockNotHeldException G-Research#65
@georgy93 mentions
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few remarks regarding the test code.
|
||
var distributedLock = _client.CreateLock(new LockOptions(keyName) | ||
{ | ||
SessionTTL = TimeSpan.FromSeconds(DefaultSessionTTLSeconds), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that defining these values as constants improves the test readability, but I'm not against it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me either way. #67 uses them too
Hi @georgy93, could you please review this PR and confirm this is your actual use case? |
Let's merge this? @marcin-krystianc @jgiannuzzi |
Fixes part of #65 (should be split in 2 issues).
This resulted in a NullReferenceException hiding the expected (?) LockNotHeldException.