Unnecessary delay in LockTryOnce scenario about consuldotnet HOT 10 CLOSED

I think I shot myself in the foot when I was writing this with a configurable LockRetryTime, which is why the check is there. We'd be checking for elapsed twice in the case of retry time, and that doesn't really prevent a hot loop, which is very possible and tips over your Consul agent, if not cluster, so you really don't want one.

What you said though is a restatement with different words of what I said, which means we're in agreement, though!

I said "if (!(Opts.LockTryOnce && Opts.LockWaitTime == Timespan.Zero)) means that they really mean don't wait, so don't respect retry time OR accept a zero retry time." This is equivalent to what you said in with "Maybe we could instead accept a zero LockRetryTime when LockWaitTime == Timespan.Zero and LockTryOnce == true?"

Wait time is just how long it took for the HTTP call to come back and should not be removed from Retry time though. It doesn't matter if retry time is 500ms and wait time is 5s and you waited 3.7s. You should make another call at 4.2s after the first, because it's when the return happened that mattered, not when you started the call. If you called again immediately, at that point if you're making a lot of calls close together, which is what the retry timer is trying to prevent.

If we want to allow the caller to possibly footgun their Consul cluster (which I'm in favor of!), then we should go with accepting an explicit LockRetryTime = Timespan.Zero, only if LockWaitTime == Timespan.Zero and LockTryOnce == true. They're all in the same options class so it should be okay. A hot loop should never be implicit IMO.

from consuldotnet.

Thanks for your interest in Consul.NET @maxoris, and sorry for the very long delay in replying! (You could call it an unnecessary delay as well 😅)

I would like to clarify the issue you are facing.
If the lock is already held when trying to acquire it, then setting LockTryOnce = true and LockWaitTime = TimeSpan.FromTicks(0) will fail immediately with LockMaxAttemptsReachedException. If instead a lock delay is in effect (see https://www.consul.io/docs/internals/sessions.html), and whether LockWaitTime is zero or non-zero, then indeed we will wait for LockRetryTime and then fail (unless the lock was released in the meantime).

A fix would need to check whether LockTryOnce is false and whether the elapsed time is greater than LockWaitTime before waiting for LockRetryTime. If we don't do that, I think we might introduce a hot loop.

I will review your PR #21, but I would like to ask @highlyunavailable for her valuable opinion before going further.

from consuldotnet.

This code was directly ported from the Consul Go code, so many idiosyncrasies of the Go code were inherited. You can see the current behavior in said code matches this. Also as noted, as it stands now, the code in #21 actually makes a hot loop happen if LockTryOnce isn't set.

That said I actually think this is not a correct change and LockWaitTime = TimeSpan.Zero (or FromTicks(0)) should be used as an option instead.

The reason for this is that the Consul API uses HTTP long polls. These can be made a defined time with a "WaitTime" argument on the client, which causes the Consul Agent side to return data via HTTP response after WaitTime ms. This causes an return on the API.

LockTryOnce is a misnomer. What this code actually does is attempt to acquire a lock within a timespan. It's exactly analogous to SemaphoreSlim.Wait(Timespan timeout). Under the hood, it attempts to acquire the lock multiple times if needed (due to the HTTP Long Poll returning early), and will do so as many times as it can within the bounds set by LockWaitTime.

In the extremely exact case, we could possibly allow a condition of if (!(Opts.LockTryOnce && Opts.LockWaitTime == Timespan.Zero)) which would truly be an invariant case of never being able to try again, but I would insist on explicit tests around that case as well. A LockWaitTime of 0 translates to a QueryOptions.WaitTime of 0 which means "immediately return" so the long poll wouldn't be in effect any longer and thus it'd be okay to skip the hot loop wait.

from consuldotnet.

Thank you very much for your insightful feedback, @highlyunavailable!

Given that the current behaviour of the Consul Go client is similar, I wonder whether we might want to keep this as-is?

@maxoris Does setting LockWaitTime = TimeSpan.Zero and LockRetryTime = TimeSpan.FromMilliseconds(500) work for you? In that case, you'll either acquire immediately the lock, or wait 500ms in case of lock delay.

from consuldotnet.

You could even set LockRetryTime = Timespan.Zero if you want. That means it'll truly try once and never again, and immediately return if that one single try failed.

from consuldotnet.

There is a minimum value of 500ms for LockRetryTime, hence my suggestion. I suppose you added it to avoid letting users inadvertently trigger a hot loop.

from consuldotnet.

I'm betting that was also a carryover from Go. I would say we could easily accept a removal of that argument check, since it is definitely not in current Go code. I'm honestly not sure where it came from though.

from consuldotnet.

It looks like in Go they never even implemented a configurable LockRetryTime, so they can't have a hot loop.
This makes me think that removing the argument check altogether might let users shoot themselves in the foot.

Maybe we could instead accept a zero LockRetryTime when LockWaitTime == Timespan.Zero and LockTryOnce == true?
Though in that case, we might even be better off checking whether LockTryOnce == false and whether the elapsed time is greater than LockWaitTime before waiting for LockRetryTime. That way, the user's wish of setting LockTryOnce and LockWaitTime = Timespan.Zero would be fully respected without needing to know about LockRetryTime.

What do you think?

from consuldotnet.

Awesome, thanks @highlyunavailable!

@maxoris, would you like to change your PR #21 to implement what Ann described above?

from consuldotnet.

@jgiannuzzi Done.
But as mentioned above LockTryOnce is a misnomer. It's really unclear that we have to set LockWaitTime = Timespan.Zero to make it truly once. I would suggest to mark it as obsolete in future releases.

@highlyunavailable @jgiannuzzi Thank you for your feedback!

from consuldotnet.