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

x/net/http2: add Transport.WriteByteTimeout #48830

Closed
neild opened this issue Oct 6, 2021 · 5 comments
Closed

x/net/http2: add Transport.WriteByteTimeout #48830

neild opened this issue Oct 6, 2021 · 5 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Proposals (old)
Milestone
Backlog

Comments

@neild
Copy link
Contributor

neild commented Oct 6, 2021

There are currently two ways to detect a broken HTTP/2 connection:

  • If a Read or Write to the net.Conn fails.
  • If no frames are received from the connection for ReadIdleTimeout, and no response is received from the subsequent ping within PingTimeout.

Pings are a reliable way to detect a broken connection (with the fix for #48810), but there are times when it would be nice to passively detect an unresponsive peer without needing to actively ping them.

I propose adding an additional knob to http2.Transport:

// WriteTimeout is the timeout after which the connection will be closed
// if no data can be written to it. The timeout begins when data is written
// to the connection, and is extended if any bytes can be written.
// If zero, no write timeout is set.
WriteTimeout time.Duration

Unlike ReadIdleTimeout, WriteTimeout will apply only at the time we try to write to a connection. Unlike per-request timeouts, WriteTimeout applies to the connection as a whole.
@gopherbot gopherbot added this to the Proposal milestone Oct 6, 2021
@gopherbot
Copy link

Change https://golang.org/cl/354431 mentions this issue: http2: add Transport.WriteTimeout

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Oct 13, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 13, 2021
@neild
Copy link
Contributor Author

neild commented Oct 13, 2021

Perhaps a better name is (http2.Transport).WriteByteTimeout, since http.Server has a WriteTimeout field which applies to writing an entire response.

@neild neild mentioned this issue Oct 13, 2021
Closed
@rsc rsc changed the title proposal: x/net/http2: add Transport.WriteTimeout proposal: x/net/http2: add Transport.WriteByteTimeout Oct 20, 2021
@rsc rsc moved this from Active to Likely Accept in Proposals (old) Oct 20, 2021
@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Oct 27, 2021
@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: x/net/http2: add Transport.WriteByteTimeout x/net/http2: add Transport.WriteByteTimeout Oct 27, 2021
@rsc rsc modified the milestones: Proposal, Backlog Oct 27, 2021
@chrisd8088 chrisd8088 mentioned this issue Apr 26, 2022
Merged
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
@neild
http2: add Transport.WriteByteTimeout
69f624d 
Add a Transport-level knob to set a timeout for writes to net.Conns.
If a write exceeds the timeout without making any progress (at least
one byte written), the connection is closed.

Fixes golang/go#48830.

Change-Id: If0f57996d11c92bced30e07d1e238cbf8994acb4
Reviewed-on: https://go-review.googlesource.com/c/net/+/354431
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Oct 29, 2022
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 Proposal-FinalCommentPeriod
Projects
No open projects
Proposals (old)
Accepted
Milestone
Backlog
Development

No branches or pull requests
3 participants
@neild @rsc @gopherbot