Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Proposal: WinHttpHandler.TcpKeepAlive #44025

Closed
antonfirsov opened this issue Oct 29, 2020 · 7 comments · Fixed by #45494
Closed

API Proposal: WinHttpHandler.TcpKeepAlive #44025

antonfirsov opened this issue Oct 29, 2020 · 7 comments · Fixed by #45494
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http
Milestone

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Oct 29, 2020

Background and Motivation

Starting from version 20H1 (May 2020 Update) WinHTTP exposes an option (WINHTTP_OPTION_TCP_KEEPALIVE) to control TCP keepalive parameters in a way similar to the corresponding IOCTL option in Winsock:
https://docs.microsoft.com/en-us/windows/win32/winsock/sio-keepalive-vals#remarks

We have a customer request to expose this option in a managed API over WinHttpHandler (goal: meet scalability needs). This would also close a feature gap for users migrating from Framework & HttpWebRequest, where it was possible to control this parameter through ServicePoint.SetTcpKeepAlive.

Proposed API

// Using a class, so we can set default values for properties
// The verbose name is here to avoid potential future conflicts 
// with a TcpKeepAlive type we may define in System.Net.Sockets
public class WinHttpTcpKeepAlive
{
    // Must be greater than TimeSpan.Zero
    // Hard coded default value: 2 hours (based on Windows default)
    public TimeSpan KeepAliveTime { get; set; }

    // Must be greater than TimeSpan.Zero
    // Hard coded default value: 1 sec (based on Windows default)
    public TimeSpan KeepAliveInterval { get; set; }
}

public class WinHttpHandler
{
#if NET5
    [SupportedOSPlatform("windows10.0.2004")]
#endif
    // If the option is not supported by the OS, WinHttpException will be thrown during FIRST REQUEST (just like with other options).
    // We may consider throwing `PlatformNotSupprtedException` below version 2004, although that would be unprecedental in WinHttpHandler
    public WinHttpTcpKeepAlive? TcpKeepAlive { get; set; }
}

Usage Examples

static HttpClient CreateHttpClient()
{
    var winHttpHandler = new WinHttpHandler()
    {
        TcpKeepAlive = new WinHttpTcpKeepAlive() 
        { 
            KeepAliveTime = TimeSpan.FromMinutes(2), 
            KeepAliveInterval = TimeSpan.FromSeconds(2)
        }
    }
    return new HttpClient(winHttpHandler);
}

Alternative Designs

  • Instead of exposing a property, expose a method with 3 arguments, like ServicePoint.SetTcpKeepAlive.

  • We may consider exposing two separate properties to match the design of the HTTP/2 timeout properties on SocketsHttpHandler (see configurable HTTP/2 PING timeouts in HttpClient #31198 (comment)), but with Winsock tcp_keepalive we are expected to set both values with the single call, so this would require us to add unnecessary arbitrary logic to the managed implementation on a place where users expect a thin wrapper over a native API.

Risks

Couldn't identify any.

@antonfirsov antonfirsov added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 29, 2020
@ghost
Copy link

ghost commented Oct 29, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@antonfirsov antonfirsov changed the title API Proposal: WinHttpHandler.SetTcpKeepAlive API Proposal: WinHttpHandler.TcpKeepAlive Oct 29, 2020
@antonfirsov antonfirsov added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 29, 2020
@bartonjs
Copy link
Member

bartonjs commented Nov 10, 2020

Video

API Review:

  • Flatten things to just the one type
  • Add a "Tcp" prefix since it's TCP KeepAlive, not HTTP KeepAlive
  • SupportedOSPlatform attributes might not be required, depending on how you handle older OS interaction
public partial class WinHttpHandler
{
#if NET5
    [SupportedOSPlatform("windows10.0.2004")]
#endif
    public bool TcpKeepAliveEnabled { get; set; }

#if NET5
    [SupportedOSPlatform("windows10.0.2004")]
#endif
    public TimeSpan TcpKeepAliveTime { get; set; }

#if NET5
    [SupportedOSPlatform("windows10.0.2004")]
#endif
    public TimeSpan TcpKeepAliveInterval { get; set; }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 10, 2020
@halter73
Copy link
Member

    // Must be greater than TimeSpan.Zero
    // Hard coded default value: 1 sec (based on Windows default)
    public TimeSpan KeepAliveInterval { get; set; }
       Keep-alive packets MUST only be sent when no data or
       acknowledgement packets have been received for the
       connection within an interval.  This interval MUST be
       configurable and MUST default to no less than two hours.

https://tools.ietf.org/html/rfc1122#page-101

A default KeepAliveInterval of 1 second which is far less than the minimum 2 hours required by the RFC.

Given servers like Kestrel and HTTP.sys have a default 2 minute HTTP keep-alive timeout, I wonder why someone would want to enable TCP keep-alive for an HTTP connection. Do we need to expose a similar option in Kestrel?

@antonfirsov
Copy link
Member Author

A default KeepAliveInterval of 1 second which is far less than the minimum 2 hours required by the RFC.

I think the referenced text talks about what became "keep alive time" (timeout before starting to exchange keepalive packets) not "keep alive interval" in the implementation.

I wonder why someone would want to enable TCP keep-alive for an HTTP connection

The reason is probably that HTTP keepalive only works with HTTP2.

@halter73
Copy link
Member

The reason is probably that HTTP keepalive only works with HTTP2.

I'm not referring to HTTP/2 pings. I'm referring the the behavior of most HTTP servers which will close HTTP connections if no requests are made within a specified interval. This interval is 2 minutes by default for Kestrel and HTTP.sys.

Regardless of TCP keep-alives, servers will close the connection anyway in far less than two hours. If the concern is about what happens if the connection is idle mid-request, servers should close connections that are not uploading request data at a sufficient rate regardless of TCP keep-alives. If the request is upgraded for WebSockets which can be idle, the client can use WebSocket keep-alives.

@halter73
Copy link
Member

halter73 commented Nov 11, 2020

I think the referenced text talks about what became "keep alive time" (timeout before starting to exchange keepalive packets) not "keep alive interval" in the implementation.

Good point. Since that section started out talking about when keep-alive packets should be sent, I misinterpreted it.

Edit: Actually, are you sure about this? I'm still reading this as the minimum default time period without receiving packets before a keep-alive packet can be sent. I did see that 1 second is indeed the documented Windows default though.

@antonfirsov
Copy link
Member Author

Regardless of TCP keep-alives, servers will close the connection anyway in far less than two hours.

According to my (probably incomplete) understanding: what customers want here is to both overwrite the quoted behavior and also make sure the TCP connections remain open via keepalive.

I'm still reading this as the minimum default time period without receiving packets before a keep-alive packet can be sent. I did see that 1 second is indeed the documented Windows default though.

I think the original intent of the spec doesn't really matter (especially in the context of this proposal) if winsock implemented it differently. The proposed API is platform-specific.

antonfirsov added a commit that referenced this issue Nov 25, 2020
Implements the final version of the API proposal in #44025 except the [SupportedOSPlatform("windows10.0.2004")] bits
@ghost ghost locked as resolved and limited conversation to collaborators Jan 7, 2021
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http
Projects
None yet
5 participants