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

proposal: add httputil.TCPKeepAliveListener #23378

Closed
kevinburke opened this issue Jan 8, 2018 · 12 comments

Comments

Projects
None yet
8 participants
@kevinburke
Copy link
Contributor

commented Jan 8, 2018

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.

@gopherbot gopherbot added this to the Proposal milestone Jan 8, 2018

@gopherbot gopherbot added the Proposal label Jan 8, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 8, 2018

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

Or do this by default. #23459.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

On hold for #23459.

@rsc rsc added the Proposal-Hold label Jan 22, 2018

@SteelPhase

This comment has been minimized.

Copy link

commented Mar 20, 2019

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

@cespare

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@bradfitz sure, will do.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 3, 2019

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

@costela

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@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.

@gopherbot gopherbot closed this in 1abf3aa Apr 4, 2019

@cespare

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@costela thanks!

@CAFxX

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@CAFxX yeah, open a new issue.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.