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

Jump to bottom

proposal: add httputil.TCPKeepAliveListener #23378

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

proposal: add httputil.TCPKeepAliveListener #23378

kevinburke opened this issue Jan 8, 2018 · 15 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted
Milestone
Proposal

Comments

@kevinburke
Copy link
Contributor

kevinburke commented Jan 8, 2018 •
edited

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
@bradfitz
Copy link
Contributor

bradfitz commented Jan 8, 2018

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

@rsc
Copy link
Contributor

rsc commented Jan 22, 2018

Or do this by default. #23459.

@rsc
Copy link
Contributor

rsc commented Jan 22, 2018

On hold for #23459.

@SteelPhase
Copy link

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

@cespare
Copy link
Contributor

cespare 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
Copy link
Contributor

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
Copy link
Contributor

cespare commented Mar 28, 2019

@bradfitz sure, will do.

@costela costela mentioned this issue Apr 3, 2019
Closed
@gopherbot
Copy link

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

@costela
Copy link
Contributor

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

@cespare
Copy link
Contributor

cespare commented Apr 6, 2019

@costela thanks!

@CAFxX
Copy link
Contributor

CAFxX commented Apr 17, 2019 •
edited

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
Copy link
Contributor

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

@CAFxX CAFxX mentioned this issue Apr 17, 2019
Closed
@rsc rsc reopened this Jun 27, 2019
@rsc
Copy link
Contributor

rsc commented Jun 27, 2019

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

Are we OK with this API?

@rsc rsc mentioned this issue Jun 27, 2019
Closed
@rsc
Copy link
Contributor

rsc commented Jun 27, 2019

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

@rsc rsc closed this as completed Jun 27, 2019
@networkimprov
Copy link

networkimprov commented Jul 16, 2019 •
edited

FYI, the new patch triggers this bug: #31449

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

@networkimprov networkimprov mentioned this issue Jul 24, 2019
Closed
@golang golang locked and limited conversation to collaborators Oct 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
9 participants
@bradfitz @costela @rsc @kevinburke @cespare @CAFxX @networkimprov @SteelPhase @gopherbot