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

@@ -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,10 +177,15 @@ WAIT:
 	default:
 	}
 
-	// See if we completed a one-shot.
+	// Handle the one-shot mode.
-	if attempts > 0 && l.opts.LockTryOnce {
+	if l.opts.LockTryOnce && attempts > 0 {
+		elapsed := time.Now().Sub(start)
+		if elapsed > qOpts.WaitTime {
 			return nil, nil
 		}
+
+		qOpts.WaitTime -= elapsed
+	}
 	attempts++
 
 	// Look for an existing lock, blocking until not taken

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 {
+	diff := time.Now().Sub(start)
-		t.Fatalf("took too long: %9.6f", diff.Seconds())
+	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.

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,10 +196,15 @@ WAIT:
 	default:
 	}
 
-	// See if we completed a one-shot.
+	// Handle the one-shot mode.
-	if attempts > 0 && s.opts.SemaphoreTryOnce {
+	if s.opts.SemaphoreTryOnce && attempts > 0 {
+		elapsed := time.Now().Sub(start)
+		if elapsed > qOpts.WaitTime {
 			return nil, nil
 		}
+
+		qOpts.WaitTime -= elapsed
+	}
 	attempts++
 
 	// Read the prefix

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 {
+	diff := time.Now().Sub(start)
-		t.Fatalf("took too long: %9.6f", diff.Seconds())
+	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.

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
+  -try=timeout               Attempt to acquire the lock up to the given
-                             up to the given duration (eg. "15s").
+                             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 {
+		if wait <= 0 {
-			c.Ui.Error("Duration for 'try' option must be positive")
+			c.Ui.Error("Try timeout must be positive")
 			return 1
 		}
 

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=blah", "test/prefix", "date"}, "parsing try timeout")
-	argFail(t, []string{"-try=-10s", "test/prefix", "date"}, "must be positive")
+	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")
 }
 

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
+* `-try` - Attempt to acquire the lock up to the given timeout. The timeout is a
-  duration supplied as the argument. The duration is a decimal number, with
+  positive decimal number, with unit suffix, such as "500ms". Valid time units
-  unit suffix, such as "500ms". Valid time units are "ns", "us" (or "µs"), "ms",
+  are "ns", "us" (or "µs"), "ms", "s", "m", "h".
-  "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