Improve keep-alive timeout #1102
Conversation
| StopAsync(); | ||
| } | ||
|
|
||
| _secondsSinceLastConnectionEvent++; |
There was a problem hiding this comment.
This relies on Tick() being called exactly once per second, which seems fragile. Have you compared the perf of something like System.Diagnostics.Stopwatch(), which would basically track the precise time of the last connection event, rather than an integer number of seconds?
There was a problem hiding this comment.
It hadn't occurred to me to use a Stopwatch to track the time between ticks - I'll try that out. When I first wrote this I did try 3 different ways to track time more accurately but they all had significant perf costs.
There was a problem hiding this comment.
Looking at how Stopwatch is implemented (https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/src/System/Diagnostics/Stopwatch.cs), I think it'll have the same perf costs as the other stuff I did try before.
There was a problem hiding this comment.
In this case I would make a few changes:
- Track the time between connection events as milliseconds rather than seconds.
- Make the millseconds between ticks public rather than private (
KestrelThread._heartbeatMilliseconds). - Update the call to
_heartbeatTimer.Start()to use the public constant. - In
Tick(), increment the elapsed milliseconds by the public constant.
|
I must have gotten it right this time, because there's almost no perf impact with my time tracking changes (494ffde) 😁 Averages of five runs of dev vs this change, difference in numbers is due to noise across runs: dev: 1130819.484 RPS |
| _frame.Tick(); | ||
| if (_timeoutEnabled) | ||
| { | ||
| if (_millisecondsSinceLastConnectionEvent > ServerOptions.Limits.ConnectionTimeout.TotalMilliseconds) |
There was a problem hiding this comment.
Store this so ms doesn't need to be recalculated. You could also add a second to the configuration since _millisecondsSinceLastConnectionEvent could be up to a second too large.
| /// </summary> | ||
| /// <remarks> | ||
| /// Defaults to 2 minutes. Timeout granularity is in seconds. Sub-second values will be rounded to the next second. | ||
| /// </remarks> | ||
| public TimeSpan KeepAliveTimeout | ||
| public TimeSpan ConnectionTimeout |
There was a problem hiding this comment.
I would rather keep this a timeout for only time spent between request. Having this timeout trigger because an app takes a long time doing business logic before writing a response or completing seems bad to me (even if it would never kill the connection mid request or response).
cesarblum
changed the title
cesarblum
changed the title
| void IConnectionControl.SetTimeout(long milliseconds) | ||
| { | ||
| // Add KestrelThread.HeartbeatMilliseconds extra milliseconds since this can be called right before the next heartbeat. | ||
| Interlocked.Exchange(ref _timeoutTimestamp, _lastTimestamp + milliseconds + KestrelThread.HeartbeatMilliseconds); |
There was a problem hiding this comment.
Can we have a Debug.Assert or something verifying that the _timeoutTimestamp was long.MaxValue previously?
| mockLibuv.ReadCallback(socket.InternalGetHandle(), 0, ref ignored); | ||
| Assert.False(connection.SocketInput.CheckFinOrThrow()); | ||
| Connection connection = null; | ||
| await context.Thread.PostAsync(_ => |
There was a problem hiding this comment.
Why does this need to be changed now?
There was a problem hiding this comment.
Because Connection's ctor calls UvLoopHandle.Now(), so it needs to run in the same thread the loop was created.
|
I like this sooo much better. Nice 👍 |
- Track time more accurately - Control timeout in Connection instead of Frame
cesarblum
force-pushed
the
cesarbs/connection-timeout
branch
from
8cd46b3 to
1a273f5
Compare
Connectioninstead ofFrame@halter73 @mikeharder @davidfowl