Add a timeout for draining requests on shutdown #300
Conversation
| @@ -88,6 +88,30 @@ public class ServerTests | |||
| } | |||
|
|
|||
| [ConditionalFact] | |||
There was a problem hiding this comment.
Unrelated to the change itself, but these are conditional upon nothing. Change to Fact.
There was a problem hiding this comment.
| @@ -88,6 +88,30 @@ public class ServerTests | |||
| } | |||
|
|
|||
| [ConditionalFact] | |||
| public async Task Server_ShutdownDurringLongRunningRequest_TimesOut() | |||
| responseTask = SendRequestAsync(address); | ||
| Assert.True(received.WaitOne(TimeSpan.FromSeconds(10))); | ||
| } | ||
| Assert.False(shutdown.HasValue); |
There was a problem hiding this comment.
Check the logs written and make sure there's one request outstanding?
There was a problem hiding this comment.
redundant. I know there's an outstanding request because of received and shutdown
| @@ -88,6 +88,30 @@ public class ServerTests | |||
| } | |||
|
|
|||
| [ConditionalFact] | |||
| public async Task Server_ShutdownDurringLongRunningRequest_TimesOut() | |||
Tratcher
force-pushed
the
tratcher/shutdown
branch
from
60d5114 to
56bd85a
Compare
| } | ||
| Assert.False(shutdown.HasValue); | ||
| waitForShutdown.Set(); | ||
| await Assert.ThrowsAsync<HttpRequestException>(async () => await responseTask); |
There was a problem hiding this comment.
Are there any specific things you can check in the exception to make sure it failed for the right reason?
There was a problem hiding this comment.
Not consistently across net451 and core.
| @@ -81,6 +81,12 @@ public long RequestQueueLimit | |||
| } | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// The amount of time to wait for active requests to drain while the server is shutting down. | |||
| /// New requests will receive a 503 response in this time period. The default is 5 seconds. | |||
There was a problem hiding this comment.
I think the default info should be in <remarks>.
There was a problem hiding this comment.
remarks looks like it's for extended discussion. summary is for IntelliSense. The comment is short enough we don't need to split it.
There was a problem hiding this comment.
Why does Kestrel use remarks so much?
#298
Using the same option name and default as Kestrel.