From 49342dc973278fb9a444321bae3cd1d906fd636f Mon Sep 17 00:00:00 2001 From: James Phillips Date: Wed, 6 Jan 2016 09:40:20 -0800 Subject: [PATCH] Makes the timeout behavior more intuitive. Previously, it would try once "up to" the timeout, but in practice it would just fall through. This modifies the behavior to block until the timeout has been reached. --- api/lock.go | 12 +++++++++--- api/lock_test.go | 5 +++-- api/semaphore.go | 12 +++++++++--- api/semaphore_test.go | 5 +++-- command/lock.go | 10 +++++----- command/lock_test.go | 5 +++-- website/source/docs/commands/lock.html.markdown | 7 +++---- 7 files changed, 35 insertions(+), 21 deletions(-) diff --git a/api/lock.go b/api/lock.go index 0882e520d2..fafe8b5259 100644 --- a/api/lock.go +++ b/api/lock.go @@ -167,6 +167,7 @@ func (l *Lock) Lock(stopCh <-chan struct{}) (<-chan struct{}, error) { WaitTime: l.opts.LockWaitTime, } + start := time.Now() attempts := 0 WAIT: // Check if we should quit @@ -176,9 +177,14 @@ WAIT: default: } - // See if we completed a one-shot. - if attempts > 0 && l.opts.LockTryOnce { - return nil, nil + // Handle the one-shot mode. + if l.opts.LockTryOnce && attempts > 0 { + elapsed := time.Now().Sub(start) + if elapsed > qOpts.WaitTime { + return nil, nil + } + + qOpts.WaitTime -= elapsed } attempts++ diff --git a/api/lock_test.go b/api/lock_test.go index fe279d803c..5cc74e19c3 100644 --- a/api/lock_test.go +++ b/api/lock_test.go @@ -541,8 +541,9 @@ func TestLock_OneShot(t *testing.T) { if ch != nil { t.Fatalf("should not be leader") } - if diff := time.Now().Sub(start); diff > 2*contender.opts.LockWaitTime { - t.Fatalf("took too long: %9.6f", diff.Seconds()) + diff := time.Now().Sub(start) + if diff < contender.opts.LockWaitTime || diff > 2*contender.opts.LockWaitTime { + t.Fatalf("time out of bounds: %9.6f", diff.Seconds()) } // Unlock and then make sure the contender can get it. diff --git a/api/semaphore.go b/api/semaphore.go index ab01773f58..2152ead203 100644 --- a/api/semaphore.go +++ b/api/semaphore.go @@ -186,6 +186,7 @@ func (s *Semaphore) Acquire(stopCh <-chan struct{}) (<-chan struct{}, error) { WaitTime: s.opts.SemaphoreWaitTime, } + start := time.Now() attempts := 0 WAIT: // Check if we should quit @@ -195,9 +196,14 @@ WAIT: default: } - // See if we completed a one-shot. - if attempts > 0 && s.opts.SemaphoreTryOnce { - return nil, nil + // Handle the one-shot mode. + if s.opts.SemaphoreTryOnce && attempts > 0 { + elapsed := time.Now().Sub(start) + if elapsed > qOpts.WaitTime { + return nil, nil + } + + qOpts.WaitTime -= elapsed } attempts++ diff --git a/api/semaphore_test.go b/api/semaphore_test.go index ab296c4beb..4cec164f4e 100644 --- a/api/semaphore_test.go +++ b/api/semaphore_test.go @@ -499,8 +499,9 @@ func TestSemaphore_OneShot(t *testing.T) { if ch != nil { t.Fatalf("should not have acquired the semaphore") } - if diff := time.Now().Sub(start); diff > 2*contender.opts.SemaphoreWaitTime { - t.Fatalf("took too long: %9.6f", diff.Seconds()) + diff := time.Now().Sub(start) + if diff < contender.opts.SemaphoreWaitTime || diff > 2*contender.opts.SemaphoreWaitTime { + t.Fatalf("time out of bounds: %9.6f", diff.Seconds()) } // Give up a slot and make sure the third one can get it. diff --git a/command/lock.go b/command/lock.go index 049d2cbc21..72f4ec5830 100644 --- a/command/lock.go +++ b/command/lock.go @@ -75,8 +75,8 @@ Options: -name="" Optional name to associate with lock session. -token="" ACL token to use. Defaults to that of agent. -pass-stdin Pass stdin to child process. - -try=duration Make a single attempt to acquire the lock, waiting - up to the given duration (eg. "15s"). + -try=timeout Attempt to acquire the lock up to the given + timeout (eg. "15s"). -monitor-retry=n Retry up to n times if Consul returns a 500 error while monitoring the lock. This allows riding out brief periods of unavailability without causing leader @@ -145,12 +145,12 @@ func (c *LockCommand) run(args []string, lu **LockUnlock) int { var err error wait, err = time.ParseDuration(try) if err != nil { - c.Ui.Error(fmt.Sprintf("Error parsing duration for 'try' option: %s", err)) + c.Ui.Error(fmt.Sprintf("Error parsing try timeout: %s", err)) return 1 } - if wait < 0 { - c.Ui.Error("Duration for 'try' option must be positive") + if wait <= 0 { + c.Ui.Error("Try timeout must be positive") return 1 } diff --git a/command/lock_test.go b/command/lock_test.go index a067f80af5..34c859a51f 100644 --- a/command/lock_test.go +++ b/command/lock_test.go @@ -29,8 +29,9 @@ func argFail(t *testing.T, args []string, expected string) { } func TestLockCommand_BadArgs(t *testing.T) { - argFail(t, []string{"-try=blah", "test/prefix", "date"}, "parsing duration") - argFail(t, []string{"-try=-10s", "test/prefix", "date"}, "must be positive") + argFail(t, []string{"-try=blah", "test/prefix", "date"}, "parsing try timeout") + argFail(t, []string{"-try=0s", "test/prefix", "date"}, "timeout must be positive") + argFail(t, []string{"-try=-10s", "test/prefix", "date"}, "timeout must be positive") argFail(t, []string{"-monitor-retry=-5", "test/prefix", "date"}, "must be >= 0") } diff --git a/website/source/docs/commands/lock.html.markdown b/website/source/docs/commands/lock.html.markdown index dceb4f05f4..cb016412de 100644 --- a/website/source/docs/commands/lock.html.markdown +++ b/website/source/docs/commands/lock.html.markdown @@ -62,10 +62,9 @@ The list of available flags are: * `-pass-stdin` - Pass stdin to child process. -* `-try` - Make a single attempt to acquire the lock, waiting up to the given - duration supplied as the argument. The duration is a decimal number, with - unit suffix, such as "500ms". Valid time units are "ns", "us" (or "µs"), "ms", - "s", "m", "h". +* `-try` - Attempt to acquire the lock up to the given timeout. The timeout is a + positive decimal number, with unit suffix, such as "500ms". Valid time units + are "ns", "us" (or "µs"), "ms", "s", "m", "h". * `-monitor-retry` - Retry up to this number of times if Consul returns a 500 error while monitoring the lock. This allows riding out brief periods of unavailability