Incorrect Lock Wait Time, Weird Implementation

[undecidedapollo]

This issue encompasses two bugs.

The first bug is that the wait time specified is not correct. Around line 244, the code says:

                        if (attempts > 0 && Opts.LockTryOnce)         
                        {
                            var elapsed = DateTime.UtcNow.Subtract(start);
                            if (elapsed > qOpts.WaitTime)
                            {
                                DisposeCancellationTokenSource();
                                throw new LockMaxAttemptsReachedException("LockTryOnce is set and the lock is already held or lock delay is in effect");
                            }
                            qOpts.WaitTime -= elapsed;
                        }

In this code, qOpts.WaitTime -= elapsed, the wait time is reduced from the time elapsed. This causes the time waited to be exponentially shorter than the specified wait time, depending on the number of iterations. To fix this, this line should be removed. The code already correctly checks the amount of elapsed time above.

The second bug is more of a clarity issue. In order for the specified wait time to be respected, you have to also set LockTryOnce to true. This can be seen on line 244 " if (attempts > 0 && Opts.LockTryOnce) " This is unclear behavior and should not be necessary. Instead, LockTryOnce should only try to lock it once, where as WaitTime should be respected in all scenarios, the behavior of the two variables should not be coupled.

[highlyunavailable]

Thanks for submitting this. There is a bug here, but it's not quite what you suggested.

Instead of qOpts.WaitTime -= elapsed;, that line should read qOpts.WaitTime = Opts.LockWaitTime - elapsed;

That said, I can't actually see a codepath where attempts is not incremented but the loop starts over so this is probably just artifacts from porting from the Go API: https://github.com/hashicorp/consul/blob/master/api/lock.go#L182-L190

"WaitTime" is how long the long poll waits before returning even if no changes have been set. WaitTime is already respected by the agent in the case of LockTryOnce not being set - the C# code just waits infinitely in a loop and there is no such thing as a "Try to lock and then bail out after 30 seconds if not acquired". If you want a lock timeout, it's up to you to pass in a CancellationToken to the lock acquisition and cancel that token after your timeout period if you want to cancel acquisition.