Comments (10)
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.
Related Issues (20)
- Support Applying Configuration and Get Configuration
- Support Listing and Deleting Configuration Entries
- Support for Listing Services for Node and Retrieving Nodes that support Mesh Capable Service
- Support Getting Service Configuration
- Wanna do some contribution HOT 1
- [FEATURE REQUEST]: support weights option
- Missing features between Consul.NET (v1.7.14) and target version (v1.9.17) of Consul/Official Client
- FEATURE REQUEST: Support for List Nodes for Connect-capable Service HOT 1
- FEATURE REQUEST: Read LAN Coordinates for a node HOT 1
- FEATURE REQUEST: List Services for Gateway HOT 2
- [Bug]: Automated github build of consuldotnet broken HOT 1
- FEATURE REQUEST: Update LAN Coordinates for a node HOT 2
- Add additional TFM So Microsoft.CSharp can be externalised
- [Bug]: Consul full Url is not considered due to UriBuilder and consul call fails HOT 8
- [FEATURE REQUEST]: Connect CA Roots, return root certificates in the cluster HOT 1
- [FEATURE REQUEST]: Connect CA Leaf, returns the leaf certificate representing a single service HOT 1
- [FEATURE REQUEST]: Connect Authorize, tests whether a connection attempt is authorized between two services
- Listing all services in catalog service is missing filter parameter HOT 1
- [FEATURE REQUEST]: List Ingress Health for Service
- Fix some flaky tests:
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from consuldotnet.