search

valyala / fasthttp

Fast HTTP package for Go. Tuned for high performance. Zero memory allocations in hot paths. Up to 10x faster than net/http
MIT License
21.91k stars 1.76k forks source link

How to handle TCP keepalive-related configurations more effectively? #1836

Open newacorn opened 3 months ago

newacorn commented 3 months ago

Currently, when Server.TCPKeepalive is set to false, if the net.Listener is created by the package itself, it does not override the default behavior of net.ListenConfig, which keeps TCP keepalive enabled.

I also added TCPKeepalive and TCPKeepalivePeriod fields to the client to make its behavior consistent with the server.

The goal of this pull request is to ensure that whether TCP keepalive is enabled is entirely controlled by the configuration fields provided by the package when net.TCPConn is created by the package itself. If net.TCPConn is created by the user, it means that on the server side, the user provides their own net.Listener, and on the client side, the user provides their own Dial and DialTimeout functions. The TCP keepalive mechanism is managed by the user in this case, When TCPKeepalive field is false.

If the user provides their own net.Listener, even after this pull request is accepted, the implementation of fasthttp means that Server.TCPKeepalive is only meaningful when set to true. If set to false, it does not change the existing TCP keepalive behavior of the net.TCPConn created by the user-provided net.Listener. https://github.com/valyala/fasthttp/blob/43c7b83ee54b9e78ecb79ae1a6346e9a82342f03/server.go#L1956-L1967

To avoid unnecessary and redundant system calls, I think this behavior is acceptable. At the same time add some relevant comments to make the meaning clearer.

    // Whether the operating system should send tcp keep-alive messages on the tcp connection.
    //
    // When this field is set to false, it is only effective if the net.Listener
    // is created by this package itself. i.e. listened by ListenAndServe, ListenAndServeUNIX,
    // ListenAndServeTLS and  ListenAndServeTLSEmbed method.
    //
    // If you have created a net.Listener yourself and configured TCP keepalive,
    // this field should be set to false to avoid redundant additional system calls.
    TCPKeepalive bool

The root of the problem is that the net package defaults to having TCP keepalive enabled, while the fasthttp package defaults to having TCP keepalive disabled. Clearly, the behavior of the fasthttp package is more reasonable.

The changes brought by this pull request makes the semantics of the configuration fields related to TCP keepalive clearer and more precise.

This is a preliminary personal thought; discussions are welcome.

erikdubbelboer commented 3 months ago

I'm thinking we should treat this as a backwards incompatible change as it changes the behavior of the library.

What do you think about deprecating TCPKeepalive and introducing a new TCPKeepaliveDisabled. Then the default will stay the same and upgrading won't affect any servers.

newacorn commented 3 months ago

I'm thinking we should treat this as a backwards incompatible change as it changes the behavior of the library.

What do you think about deprecating TCPKeepalive and introducing a new TCPKeepaliveDisabled. Then the default will stay the same and upgrading won't affect any servers.

I think that even with the addition of a TCPKeepaliveDisabled field, and maintaining backward compatibility, it is difficult to accurately convey the semantics. I believe that the program's runtime behavior should strictly adhere to the configuration field API. But TCP keepalive seems unlikely to affect the correctness of the program and might only have a minor impact on performance, it may be acceptable to address it in the next major release. Let's leave this issue to be resolved in the next major release.

Let's keep this pull request open; we may need more input from others.