Skip to content
This repository was archived by the owner on Mar 19, 2019. It is now read-only.

Add a timeout for draining requests on shutdown #300

Merged
merged 1 commit into from
Jan 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Microsoft.AspNetCore.Server.HttpSys/HttpSysOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default info should be in <remarks>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remarks looks like it's for extended discussion. summary is for IntelliSense. The comment is short enough we don't need to split it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does Kestrel use remarks so much?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

/// </summary>
public TimeSpan ShutdownTimeout { get; set; } = TimeSpan.FromSeconds(5);

internal void SetRequestQueueLimit(RequestQueue requestQueue)
{
_requestQueue = requestQueue;
Expand Down
10 changes: 9 additions & 1 deletion src/Microsoft.AspNetCore.Server.HttpSys/MessagePump.cs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,15 @@ public void Dispose()
if (_outstandingRequests > 0)
{
LogHelper.LogInfo(_logger, "Stopping, waiting for " + _outstandingRequests + " request(s) to drain.");
_shutdownSignal.WaitOne();
var drained = _shutdownSignal.WaitOne(Listener.Options.ShutdownTimeout);
if (drained)
{
LogHelper.LogInfo(_logger, "All requests drained successfully.");
}
else
{
LogHelper.LogInfo(_logger, "Timed out, terminating " + _outstandingRequests + " request(s).");
}
}
// All requests are finished
_listener.Dispose();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public async Task Server_EchoHelloWorld_Success()
}

[ConditionalFact]
public async Task Server_ShutdownDurringRequest_Success()
public async Task Server_ShutdownDuringRequest_Success()
{
Task<string> responseTask;
ManualResetEvent received = new ManualResetEvent(false);
Expand All @@ -87,6 +87,30 @@ public async Task Server_ShutdownDurringRequest_Success()
Assert.Equal("Hello World", response);
}

[ConditionalFact]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the change itself, but these are conditional upon nothing. Change to Fact.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public async Task Server_ShutdownDuringLongRunningRequest_TimesOut()
{
Task<string> responseTask;
var received = new ManualResetEvent(false);
bool? shutdown = null;
var waitForShutdown = new ManualResetEvent(false);
string address;
using (Utilities.CreateHttpServer(out address, httpContext =>
{
received.Set();
shutdown = waitForShutdown.WaitOne(TimeSpan.FromSeconds(15));
httpContext.Response.ContentLength = 11;
return httpContext.Response.WriteAsync("Hello World");
}))
{
responseTask = SendRequestAsync(address);
Assert.True(received.WaitOne(TimeSpan.FromSeconds(10)));
}
Assert.False(shutdown.HasValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the logs written and make sure there's one request outstanding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logs? eww

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant. I know there's an outstanding request because of received and shutdown

waitForShutdown.Set();
await Assert.ThrowsAsync<HttpRequestException>(async () => await responseTask);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any specific things you can check in the exception to make sure it failed for the right reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not consistently across net451 and core.

}

[ConditionalFact]
public void Server_AppException_ClientReset()
{
Expand Down