SocketsHttpHandler: Add response drain timeout

[geoffkizer]

We have a byte limit for response drain, but not a time limit. So a poorly behaved server could tie up the connection indefinitely. We should add a time limit.

EDIT 3/6/2018 by @stephentoub. Adding API proposal:

public int MaxResponseDrainSize { get; set; } // exists
public TimeSpan MaxResponseDrainTime { get; set; } // new (exists in implementation but as internal)

[Tratcher]

5s? Check HttpWebRequest, is has a similar timeout.

[stephentoub]

It looks like HttpWebRequest defaults to 500ms. That seems too short.

[stephentoub]

Reopening and making into an API proposal:

public int MaxResponseDrainSize { get; set; } // exists
public TimeSpan MaxResponseDrainTime { get; set; } // new

[Tratcher]

On a related note, disposing of the handler should abort all connections, not drain them.

[stephentoub]

On a related note, disposing of the handler should abort all connections, not drain them.

Are you seeing things behave otherwise?

[Tratcher]

Yes on 2.1.0-preview2-26225-03.
https://github.com/aspnet/HttpSysServer/blob/80345b7f8ffd7a2b623d2e449f6854e3037c1ccc/test/Microsoft.AspNetCore.Server.HttpSys.FunctionalTests/Listener/ResponseBodyTests.cs#L602-L615

This code gets the response without buffering, disposes it which starts a background drain that blocks without timeout, and then the client/handler is disposed. The already draining socket is not closed when the handler is disposed so the test times out.

[stephentoub]

Yes on 2.1.0-preview2-26225-03.

Ok, then please open a separate issue for that.

[terrajobst]

Looks like we want to go with these names:

public int MaxResponseDrainSize { get; set; }
public TimeSpan ResponseDrainTimeout { get; set; }