How to handle TCP keepalive-related configurations more effectively?

[newacorn]

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.

fasthttp/server.go

Lines 1956 to 1967 in 43c7b83

	if tc, ok := c.(*net.TCPConn); ok && s.TCPKeepalive {
	if err := tc.SetKeepAlive(s.TCPKeepalive); err != nil {
	_ = tc.Close()
	return nil, err
	}
	if s.TCPKeepalivePeriod > 0 {
	if err := tc.SetKeepAlivePeriod(s.TCPKeepalivePeriod); err != nil {
	_ = tc.Close()
	return nil, err
	}
	}
	}

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]

I can already see us forgetting to update this when a new version of Go is release (like we did here: https://github.com/search?q=repo%3Avalyala%2Ffasthttp+%22go%3Abuild+go1.20%22&type=code). I think it would be better to do this:

//go:build !go1.19 && !go1.20 && !go1.21 && !go1.22

That would automatically make it work for a future go1.24 etc.

[newacorn]

Sorry, I misunderstood. Go 1.23 did not change the default behavior of net.ListenConfig. If the KeepAlive field is 0, TCP keepalive will still be enabled. The new KeepAliveConfig just provides more granular control. To disable the keepalive mechanism, you still need to set the KeepAlive field to a negative value.

[newacorn]

The current build tags for Go versions seem to be correct(b2s and s2b functions). I found a related question on Stack Overflow.
Golang build tags for a particular Go version possible?

Put the code that depends on contexts in a file with the following build tag. This code is compiled for Go 1.7 and onward.
// +build go1.7
Put the backwards compatibility code in a file with the following build tag. This code is used for all versions of Go before 1.7.
// +build !go1.7

[erikdubbelboer]

You're right, I learned something new there.

[erikdubbelboer]

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]

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.