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: Client support for Expect: 100-continue #3665

Closed
bradfitz opened this Issue May 23, 2012 · 33 comments

Comments

Projects
None yet
@bradfitz
Member

bradfitz commented May 23, 2012

The http Client should probably support Expect: 100-continue.  The server does.

It does introduce a new round-trip, though, so we should probably only send it when the
data size exceeds some threshold.  Probably the same threshold we use for consuming
request bodies when a handler doesn't (maxPostHandlerReadBytes).

I think we could see if we know the size of the request body (just as NewRequest does,
checking for a few known types), but if the Body is non-nil and the ContentLength is
<=0 (unknown), we instead slurp up the Body into a bytes.Buffer to see if it's >=
someThreshold.  If so, we replace the Body with a io.MultiReader(slurped, rest) and
proceed to do a 100-continue.  If not, we blast it away HTTP/1.0-style.
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Aug 7, 2012

Member

Comment 1:

See also issue #2184, different but related.
Member

bradfitz commented Aug 7, 2012

Comment 1:

See also issue #2184, different but related.
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Sep 12, 2012

Contributor

Comment 2:

Labels changed: added priority-later, removed priority-triage.

Contributor

rsc commented Sep 12, 2012

Comment 2:

Labels changed: added priority-later, removed priority-triage.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Sep 12, 2012

Contributor

Comment 3:

Labels changed: added go1.1.

Contributor

rsc commented Sep 12, 2012

Comment 3:

Labels changed: added go1.1.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 9, 2012

Contributor

Comment 4:

Labels changed: added go1.1maybe, removed go1.1.

Contributor

rsc commented Dec 9, 2012

Comment 4:

Labels changed: added go1.1maybe, removed go1.1.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Dec 14, 2012

Member

Comment 5:

Adding the "Suggested" label, if somebody wants to take this on.  I expect it to be
somewhat involved, though.
It should probably be opt-in:  users will need to add "Expect": "100-continue" to their
request before sending it.
And then in transport.go, when we're going to write the request, write and flush the
headers, create a channel, and then wait on the channel response and a 1 second timer
(like curl).  If after 1 second no response, send the body anyway.  If the channel gives
you a non-100 HTTP response, use that.  If the channel gives a 100, start writing the
body.

Labels changed: added suggested, size-l.

Member

bradfitz commented Dec 14, 2012

Comment 5:

Adding the "Suggested" label, if somebody wants to take this on.  I expect it to be
somewhat involved, though.
It should probably be opt-in:  users will need to add "Expect": "100-continue" to their
request before sending it.
And then in transport.go, when we're going to write the request, write and flush the
headers, create a channel, and then wait on the channel response and a 1 second timer
(like curl).  If after 1 second no response, send the body anyway.  If the channel gives
you a non-100 HTTP response, use that.  If the channel gives a 100, start writing the
body.

Labels changed: added suggested, size-l.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Feb 20, 2013

Member

Comment 6:

Owner changed to ---.

Member

bradfitz commented Feb 20, 2013

Comment 6:

Owner changed to ---.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Mar 12, 2013

Contributor

Comment 7:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

Contributor

rsc commented Mar 12, 2013

Comment 7:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz
Member

bradfitz commented Mar 30, 2013

Comment 8:

Sent out https://golang.org/cl/8166045
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Mar 30, 2013

Member

Comment 9:

This issue was updated by revision a79df7b.

R=golang-dev, dsymonds, dave, jgrahamc
CC=golang-dev
https://golang.org/cl/8166045
Member

bradfitz commented Mar 30, 2013

Comment 9:

This issue was updated by revision a79df7b.

R=golang-dev, dsymonds, dave, jgrahamc
CC=golang-dev
https://golang.org/cl/8166045
@andybalholm

This comment has been minimized.

Show comment
Hide comment
@andybalholm

andybalholm Mar 30, 2013

Comment 10:

I think this fixes issue #2184.

andybalholm commented Mar 30, 2013

Comment 10:

I think this fixes issue #2184.
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jul 30, 2013

Contributor

Comment 11:

Labels changed: added go1.2maybe.

Contributor

rsc commented Jul 30, 2013

Comment 11:

Labels changed: added go1.2maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jul 30, 2013

Contributor

Comment 12:

Labels changed: added feature.

Contributor

rsc commented Jul 30, 2013

Comment 12:

Labels changed: added feature.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 29, 2013

Contributor

Comment 13:

Won't happen for 1.2.

Labels changed: removed go1.2maybe.

Contributor

robpike commented Aug 29, 2013

Comment 13:

Won't happen for 1.2.

Labels changed: removed go1.2maybe.

@mattetti

This comment has been minimized.

Show comment
Hide comment
@mattetti

mattetti Oct 13, 2013

Contributor

Comment 14:

Just a quick note to let you know that node. Is only send a 100 if the request sends an
expect 100. That seems like a good idea.
Also, does the http client supports expect 100 with timeout
Contributor

mattetti commented Oct 13, 2013

Comment 14:

Just a quick note to let you know that node. Is only send a 100 if the request sends an
expect 100. That seems like a good idea.
Also, does the http client supports expect 100 with timeout
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 27, 2013

Contributor

Comment 15:

Labels changed: added go1.3maybe.

Contributor

rsc commented Nov 27, 2013

Comment 15:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Nov 27, 2013

Contributor

Comment 16:

Labels changed: removed feature.

Contributor

rsc commented Nov 27, 2013

Comment 16:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 17:

Labels changed: added release-none, removed go1.3maybe.

Contributor

rsc commented Dec 4, 2013

Comment 17:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Dec 4, 2013

Contributor

Comment 18:

Labels changed: added repo-main.

Contributor

rsc commented Dec 4, 2013

Comment 18:

Labels changed: added repo-main.

@jbardin

This comment has been minimized.

Show comment
Hide comment
@jbardin

jbardin Mar 31, 2014

Contributor

Comment 19:

I was going create a patch for this, but noticed that the concurrent read/write loops
handle this surprisingly well as is. A very easy improvement would be to flush headers
immediately if expectsContinue during the initial write.
One question regarding brafitz's earlier comment, do we actually *need* to block sending
the body (seems ambiguous in the spec). I'd actually prefer the current behavior with
the added flush, as it would get a head start on sending data, and it looks like the
client still shuts down immediately upon a non 100-continue response.
Contributor

jbardin commented Mar 31, 2014

Comment 19:

I was going create a patch for this, but noticed that the concurrent read/write loops
handle this surprisingly well as is. A very easy improvement would be to flush headers
immediately if expectsContinue during the initial write.
One question regarding brafitz's earlier comment, do we actually *need* to block sending
the body (seems ambiguous in the spec). I'd actually prefer the current behavior with
the added flush, as it would get a head start on sending data, and it looks like the
client still shuts down immediately upon a non 100-continue response.
@anacrolix

This comment has been minimized.

Show comment
Hide comment
@anacrolix

anacrolix May 10, 2015

Contributor

I believe this is the cause for issues I'm having PUT-ing to a server that redirects me. The body in the initial PUT request is prematurely consumed, despite my having set Expect: 100-continue. If there is Expect: 100-continue in the request, the body needs to be withheld from the request until a 100 Continue is received from the server.

Contributor

anacrolix commented May 10, 2015

I believe this is the cause for issues I'm having PUT-ing to a server that redirects me. The body in the initial PUT request is prematurely consumed, despite my having set Expect: 100-continue. If there is Expect: 100-continue in the request, the body needs to be withheld from the request until a 100 Continue is received from the server.

@matope

This comment has been minimized.

Show comment
Hide comment
@matope

matope May 14, 2015

Contributor

Although I'm not sure if it were a right implementation, I've sent my small patch for this issue.
https://go-review.googlesource.com/#/c/10091/

I hit this premature consuming issue on my project. So I would appreciate if someone review this CL.

Contributor

matope commented May 14, 2015

Although I'm not sure if it were a right implementation, I've sent my small patch for this issue.
https://go-review.googlesource.com/#/c/10091/

I hit this premature consuming issue on my project. So I would appreciate if someone review this CL.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot commented May 14, 2015

CL https://golang.org/cl/10091 mentions this issue.

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn
Member

mattn commented May 29, 2015

@bradfitz ping

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 29, 2015

Member

The Go tree is currently in freeze for the Go 1.5 release. The freeze started on May 1st, and this was sent after May 1st, and this was not tagged as a Go 1.5-blocking issue.

As such, this has to wait until Go 1.6.

I've marked that CL with R=close so it doesn't show up on our dashboard, but pinging this thread at any time will make it reappear. Please reply to this bug after August 1st and we can begin the code review.

Thanks for working on this, though!

Member

bradfitz commented May 29, 2015

The Go tree is currently in freeze for the Go 1.5 release. The freeze started on May 1st, and this was sent after May 1st, and this was not tagged as a Go 1.5-blocking issue.

As such, this has to wait until Go 1.6.

I've marked that CL with R=close so it doesn't show up on our dashboard, but pinging this thread at any time will make it reappear. Please reply to this bug after August 1st and we can begin the code review.

Thanks for working on this, though!

@bradfitz bradfitz modified the milestones: Go1.6, Unplanned May 29, 2015

@matope

This comment has been minimized.

Show comment
Hide comment
@matope

matope May 29, 2015

Contributor

@bradfiz OK, I catch on your timeline. I'll ping you again this August!

@mattn Thanks for your pinging!

Contributor

matope commented May 29, 2015

@bradfiz OK, I catch on your timeline. I'll ping you again this August!

@mattn Thanks for your pinging!

@rhinoman

This comment has been minimized.

Show comment
Hide comment
@rhinoman

rhinoman Jul 31, 2015

I've been having issues with this as welI (using Go with CouchDB: http://stackoverflow.com/questions/30541591/large-put-requests-from-go-to-couchdb ) and I really hope this gets in sometime so I can remove the ugly hack-around in my code 👍

rhinoman commented Jul 31, 2015

I've been having issues with this as welI (using Go with CouchDB: http://stackoverflow.com/questions/30541591/large-put-requests-from-go-to-couchdb ) and I really hope this gets in sometime so I can remove the ugly hack-around in my code 👍

@drnic

This comment has been minimized.

Show comment
Hide comment
@drnic

drnic Aug 24, 2015

For golang-backed servers, is there a way to increase the max size before it starts the 100-continue protocol?

drnic commented Aug 24, 2015

For golang-backed servers, is there a way to increase the max size before it starts the 100-continue protocol?

@drnic

This comment has been minimized.

Show comment
Hide comment
@drnic

drnic Aug 24, 2015

My body seems to be only 2000+ characters of JSON, so am not sure what triggers to 100-continue sequence (and causes my golang client to hang until timeout)

drnic commented Aug 24, 2015

My body seems to be only 2000+ characters of JSON, so am not sure what triggers to 100-continue sequence (and causes my golang client to hang until timeout)

@anacrolix

This comment has been minimized.

Show comment
Hide comment
@anacrolix

anacrolix Aug 25, 2015

Contributor

@bradfitz can we get this in now?
On 25/08/2015 4:24 AM, "Dr Nic Williams" notifications@github.com wrote:

My body seems to be only 2000+ characters of JSON, so am not sure what
triggers to 100-continue sequence (and causes my golang client to hang
until timeout)


Reply to this email directly or view it on GitHub
#3665 (comment).

Contributor

anacrolix commented Aug 25, 2015

@bradfitz can we get this in now?
On 25/08/2015 4:24 AM, "Dr Nic Williams" notifications@github.com wrote:

My body seems to be only 2000+ characters of JSON, so am not sure what
triggers to 100-continue sequence (and causes my golang client to hang
until timeout)


Reply to this email directly or view it on GitHub
#3665 (comment).

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Aug 25, 2015

Member

I replied with some initial review feedback.

Member

bradfitz commented Aug 25, 2015

I replied with some initial review feedback.

@emicklei emicklei referenced this issue Sep 20, 2015

Closed

remove curl #2

@matope

This comment has been minimized.

Show comment
Hide comment
@matope

matope Sep 24, 2015

Contributor

@bradfitz Thank you for your feedback! I revised my patch a few weeks ago.
I think my patch got better but I'm not sure if it is enough.
If you are free, could you please look through that?

Contributor

matope commented Sep 24, 2015

@bradfitz Thank you for your feedback! I revised my patch a few weeks ago.
I think my patch got better but I'm not sure if it is enough.
If you are free, could you please look through that?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Sep 24, 2015

Member

Replied on Gerrit.

Member

bradfitz commented Sep 24, 2015

Replied on Gerrit.

@matope

This comment has been minimized.

Show comment
Hide comment
@matope

matope Oct 3, 2015

Contributor

@bradfitz Thank you for your review comments.

I've just updated my CL on Gerrit.
I fixed a issue that my previous code couldn't flush request header before waiting response.
I added some explicit codes for canceling sending body when the response status was not 100.

Contributor

matope commented Oct 3, 2015

@bradfitz Thank you for your review comments.

I've just updated my CL on Gerrit.
I fixed a issue that my previous code couldn't flush request header before waiting response.
I added some explicit codes for canceling sending body when the response status was not 100.

@bradfitz bradfitz closed this in dab143c Oct 17, 2015

@golang golang locked and limited conversation to collaborators Oct 17, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.