Go: proposal: add httputil.TCPKeepAliveListener

[kevinburke]

Currently there's no easy way for users to recreate the behavior
present in http.ListenAndServeTLS, short of copying the
tcpKeepAliveListener out of the net/http package. As a refresher, that's this:

func (srv *Server) ListenAndServeTLS(certFile, keyFile string) error {
    addr := srv.Addr
    if addr == "" {
        addr = ":https"
    }

    ln, err := net.Listen("tcp", addr)
    if err != nil {
        return err
    }

    defer ln.Close()

    return srv.ServeTLS(tcpKeepAliveListener{ln.(*net.TCPListener)}, certFile, keyFile)
}

Making that package public would make it easy for people to mirror the behavior in that function. In my case, I like to open a socket, log a message, and then start the server, as the server blocks until shutdown.

It also seems like a lot of people are trying to copy it, scan the results here:
https://github.com/search?q=tcpKeepAliveListener+language%3Ago&ref=simplesearch&type=Code&utf8=%E2%9C%93

Two questions:

• We can't load it in net/http because that would involve an import cycle. Is a copy okay?
• the keep-alive period is hard coded to 3 minutes. Should we allow users to configure this? The zero value can conceivably be valid.

[bradfitz]

Alternatively, add something to net.TCPListener so it does this automatically upon Accept.

[rsc]

Or do this by default. #23459.

[rsc]

On hold for #23459.

[SteelPhase]

Just checking in here to see, with #23459 being closed, if this can move forward or be discussed further.

[cespare]

Seems like #23459 only added a default keepalive for connections created with net.Dial(er). If we wanted to obviate this issue in a similar way, we'd need to do the analogous thing for net.Listen and friends. Am I missing something?

[bradfitz]

Yeah, we could do this by default for Listen too, but then we should add a way to disable it for people using https://golang.org/pkg/net/#ListenConfig by adding a KeepAlive field to ListenConfig like the Dialer has: https://golang.org/pkg/net/#Dialer.KeepAlive

@cespare, you want to do that?

[cespare]

@bradfitz sure, will do.

[gopherbot]

Change https://golang.org/cl/170678 mentions this issue: net: add KeepAlive field to ListenConfig

[costela]

@cespare sorry to barge in, but I took the liberty of giving this a try at #31242. I'd be greatful if you could take a look at it and see if it's anything like what you had in mind.
If you already had a CL in the works somewhere, just let me know and I'll retract mine.

[cespare]

@costela thanks!

[CAFxX]

I just stumbled upon this... I think it would be advisable, for uniformity, to use the same duration internally as the one used by Dialer (https://golang.org/src/net/dial.go#L427).

I'm not saying the value needs to be necessarily 15s (although that was debated a while back and agreed upon, see #23459), but they should be the same value (and, as discussed in #23459, I think this value should not be part of the public API).

@cespare @bradfitz WDYT? should I open a followup issue?

[cespare]

@CAFxX yeah, open a new issue.

I'd vote for doing 15s for both, for the reasons you outlined on #23459.

[rsc]

This was proposed and put on hold and then committed without the proposal being approved.

Are we OK with this API?

[rsc]

The new field in ListenConfig has the same name, type, and default as in Dialer, so approving proposal.

[networkimprov]

FYI, the new patch triggers this bug: #31449

Edit: @pam4 and I discovered that keepalive is disabled by writing to a dead link #31490