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.

Sign up for GitHub

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

Jump to bottom

Change session close logic to always attempt to close a lock session even if releasing the lock fails #126

Merged
merged 1 commit into from
Oct 28, 2021
Merged

Change session close logic to always attempt to close a lock session even if releasing the lock fails #126

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions rules/lock/lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,18 @@ var ErrNilMutex = errors.New("mutex is nil")

func (v3l *v3Lock) Unlock() error {
if v3l.mutex != nil {
// TODO: Should the timeout for this be configurable too? Or use the same value as lock?
// It's a slightly different case in that here we want to make sure the unlock
// succeeds to free it for the use of others. In the lock case we want to give up
// early if someone already has the lock.
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(5)*time.Second)
// This should be given every chance to complete, otherwise
// a lock could prevent future interactions with a resource.
ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
defer cancel()
err := v3l.mutex.Unlock(ctx)
if err == nil && v3l.session != nil {
v3l.session.Close()
// If the lock failed to be released, as least closing the session
// will allow the lease it is associated with to expire.
if v3l.session != nil {
serr := v3l.session.Close()
if err == nil {
err = serr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a new possible error, I looked at the callers, but only see logging of the error and no action/logic. This should be safe change without other new tests then.

}
}
return err
}
Expand Down