Add keep-alive timeout (#464) #1066
Conversation
| { | ||
| if ((now - _requestEndTime) / NanosecondsPerSecond > ServerOptions.Limits.KeepAliveTimeout) | ||
| { | ||
| SocketInput.IncomingComplete(0, null); |
|
Thanks @halter73 for the suggestion to count seconds between ticks instead of querying time. That fixed the perf regression 👍 |
| @@ -8,5 +8,8 @@ public interface IConnectionControl | |||
| void Pause(); | |||
| void Resume(); | |||
| void End(ProduceEndType endType); | |||
|
|
|||
| void NotifyRequestStarted(); | |||
There was a problem hiding this comment.
Why are these on IConnectionControl
There was a problem hiding this comment.
What do you suggest? To me it makes sense to expose these here - you use these to signal the connection about request processing events so it can react accordingly.
Do you think I should introduce a new interface? IRequestStatusNotification or something along those lines?
There was a problem hiding this comment.
Would it be wrong to put them straight onto connection?
There was a problem hiding this comment.
Would require Frame to know about Connection - stronger coupling.
cesarblum
force-pushed
the
cesarbs/timeouts
branch
3 times, most recently
from
f96eb6d to
f74d02a
Compare
|
⌚ |
| @@ -1267,6 +1267,25 @@ protected void ReportApplicationError(Exception ex) | |||
| Log.ApplicationError(ConnectionId, ex); | |||
| } | |||
|
|
|||
| public void Tick() | |||
| { | |||
| if (_requestProcessingStatus == RequestProcessingStatus.RequestPending && // we're in between requests and | |||
There was a problem hiding this comment.
We almost never put comments at the end of a line. I get why you did it, but just put the comment above the if and use "and" in the comment.
cesarblum
force-pushed
the
cesarbs/timeouts
branch
from
c0fd641 to
d09230e
Compare
cesarblum
force-pushed
the
cesarbs/timeouts
branch
from
9213490 to
eed9f2b
Compare
cesarblum
force-pushed
the
cesarbs/timeouts
branch
2 times, most recently
from
1ca5d62 to
bc17468
Compare
|
Travis test failure on OS X (https://travis-ci.org/aspnet/KestrelHttpServer/jobs/157181697) is not related to this change since it's showing up in other branches (https://travis-ci.org/aspnet/KestrelHttpServer/jobs/157157166, https://travis-ci.org/aspnet/KestrelHttpServer/jobs/155406648 - in the latter it was the other test in the class that failed). Still waiting for a 🚢 or more feedback. ⌚ |
| @@ -1277,7 +1295,7 @@ protected enum RequestLineStatus | |||
| Done | |||
| } | |||
|
|
|||
| protected enum RequestProcessingStatus | |||
| public enum RequestProcessingStatus | |||
There was a problem hiding this comment.
Why is this now public when the field was made private?
|
|
cesarblum
force-pushed
the
cesarbs/timeouts
branch
from
fb536d8 to
f2085b1
Compare
This includes the libuv timer stuff in #1056 plus the keep-alive code.
~~There's a ~5% perf hit. I'm trying to improve that.~~
#464
cc @halter73 @mikeharder @davidfowl @Tratcher