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

net/http: add ResponseController.EnableFullDuplex #57786

Closed
neild opened this issue Jan 13, 2023 · 14 comments
Closed

net/http: add ResponseController.EnableFullDuplex #57786

neild opened this issue Jan 13, 2023 · 14 comments

Comments

@neild
Copy link
Contributor

neild commented Jan 13, 2023

This proposal aims to address #15527.

The net/http HTTP/1 server does not permit reading from an inbound request body after starting to write the response. (See the ResponseWriter.Write documentation).

This limitation is because the server drains any unread portion of the request body before writing the response headers, to avoid deadlocking clients that attempt to write a complete request before reading the response. (See #15527 (comment) for more context.)

I propose that we offer an opt-in mechanism to disable this behavior, permitting a server handler to write some or all of the response interleaved with reads from the request.

// SetBidi indicates whether the request handler will interleave reads from Request.Body with
// writes to the ResponseWriter.
//
// For HTTP/1 requests, the Go HTTP server by default consumes any unread portion of the request
// body before beginning to write the response, preventing handlers from concurrently reading from
// the request and writing the response. Calling SetBidi(true) disables this behavior and permits
// handlers to continue to read from the request while concurrently writing the response.
//
// For HTTP/2 requests, the Go HTTP server always permits concurrent reads and responses.
func (c *ResponseController) SetBidi(bidi bool) error {}
@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

"bidi" often means bidirectional text like Unicode LTR/RTL.
It seems like a very short name for a rarely used feature.
Is there a standard name for this behavior in the HTTP specs?
It's unclear from the docs whether c.SetBidi(false) has an effect on HTTP/2 (or returns an error?).

@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

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

@neild
Copy link
Contributor Author

neild commented Feb 1, 2023

I don't believe there is a standard name for this behavior. I'm not committed to the name here; EnableInterleavedReadsAndWrites would be unambiguous if long. EnableResponseInterleaving? EnableInterleave? EnableConcurrentResponse?

SetBidi(false) will return an error for HTTP/2 requests. We already support interleaving for HTTP/2, and I don't see any value in disabling it. The reason for draining the inbound request before responding is to work better with naive clients, and a naive HTTP/2 client is somewhat of an oxymoron.

Ideally we wouldn't have a knob here, but I don't see how to avoid it; there are valid reasons to want both possible behaviors here, and changing our current default will break some users.

@seankhliao
Copy link
Member

informational rfc: bidirectional http
internet draft: full duplex

@neild
Copy link
Contributor Author

neild commented Feb 1, 2023

I like EnableFullDuplex.

And perhaps only permit enabling full duplex, to avoid any questions about what it means to disable it with HTTP/2 or HTTP/3.

func (c *ResponseController) EnableFullDuplex() error {}

@rsc rsc changed the title proposal: net/http: ResponseController.SetBidi to support concurrent Request.Body reads and ResponseWriter.Writes proposal: net/http: ResponseController.EnableFullDuplex to support concurrent Request.Body reads and ResponseWriter.Writes Feb 8, 2023
@rsc
Copy link
Contributor

rsc commented Feb 8, 2023

With the renaming to EnableFullDuplex, have all the concerns about this proposal been addressed?

@rsc rsc changed the title proposal: net/http: ResponseController.EnableFullDuplex to support concurrent Request.Body reads and ResponseWriter.Writes proposal: net/http: add ResponseController.EnableFullDuplex Feb 22, 2023
@rsc
Copy link
Contributor

rsc commented Feb 22, 2023

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

@rsc
Copy link
Contributor

rsc commented Mar 1, 2023

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: net/http: add ResponseController.EnableFullDuplex net/http: add ResponseController.EnableFullDuplex Mar 1, 2023
@rsc rsc modified the milestones: Proposal, Backlog Mar 1, 2023
@neild neild self-assigned this Mar 1, 2023
@gopherbot
Copy link

Change https://go.dev/cl/472636 mentions this issue: net/http: support full-duplex HTTP/1 responses

@gopherbot
Copy link

Change https://go.dev/cl/472717 mentions this issue: http2: support ResponseController.FullDuplex

gopherbot pushed a commit that referenced this issue Mar 7, 2023
Add support for concurrently reading from an HTTP/1 request body
while writing the response.

Normally, the HTTP/1 server automatically consumes any remaining
request body before starting to write a response, to avoid deadlocking
clients which attempt to write a complete request before reading the
response.

Add a ResponseController.EnableFullDuplex method which disables this
behavior.

For #15527
For #57786

Change-Id: Ie7ee8267d8333e9b32b82b9b84d4ad28ab8edf01
Reviewed-on: https://go-review.googlesource.com/c/go/+/472636
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/501300 mentions this issue: go1.21: document net/http.ResponseController.EnableFullDuplex

gopherbot pushed a commit that referenced this issue Jun 6, 2023
For #15527
For #57786

Change-Id: I75ed0b4bac8e31fac2afef17dad708dc9a3d74e1
Reviewed-on: https://go-review.googlesource.com/c/go/+/501300
Run-TryBot: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@francislavoie
Copy link

francislavoie commented Aug 1, 2023

Do we have a rough idea of which clients don't support full-duplex? Is there a list of clients known to produce deadlocks?

I'm asking because we're adding this as an opt-in config option in Caddy (for ref: caddyserver/caddy#5654), and I'd like it if I could document something like "don't enable this if you know you have such and such clients connecting to your server".

I'd also consider enabling this by default if it's really only super-old clients that don't handle this properly (e.g. only old browsers that nobody should be using anymore anyway, or old versions of curl, etc).

@neild
Copy link
Contributor Author

neild commented Aug 1, 2023

Sorry, I've got no idea how common this client behavior is. I wouldn't expect this to be an issue for browsers (although I haven't checked any), but browsers are also unlikely to be sending requests that need full duplex handling.

The simplest implementation of an HTTP client is to open a connection, write a request, and read a response. I suspect there are a fair number of versions of that out in the wild.

@oliverpool
Copy link

This has been implemented, documented and released in go1.21, so I guess this issue can be closed.

@neild neild closed this as completed Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

7 participants