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: Allow custom io.Readers to provide a size for ContentLength in requests #6738

Closed
cookieo9 opened this issue Nov 8, 2013 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Thinking v2 A language change or incompatible library change
Milestone

Comments

@cookieo9
Copy link
Contributor

cookieo9 commented Nov 8, 2013

Currently only 3 types can specify automatically the ContentLength of the body of an
http.Request:
 - bytes.Reader
 - bytes.Buffer
 - strings.Reader

In these cases, the *http.Request automatically has the ContentLength field set by the
value of the Len method of these three types. A user using the http package with any
other kind of io.Reader must manually call NewRequest, then set the ContentLength
themselves, and then call http.Client.Do.

If instead, if http.NewRequest was modified to support passing an io.Reader with a (for
example) Size() method, and then set the ContentLength to the value returned from that
function, users could avoid having to set the ContentLength field of the request
themselves. This would allow the http.Post() and http.Client.Post() helper methods to be
more used in more situations, if passed such an io.Reader. The bigger benefit would be
for external packages which use http though.

A situation has arisen with github package that makes http requests to the github API
where uploads must be sent a Content-Length, see:
https://groups.google.com/forum/#!topic/golang-nuts/Hipcz658QQ8

Currently the easiest way to upload files successfully is to load the entire file into a
[]byte in memory, create a bytes.Buffer or bytes.Reader from it, and pass that to the
package functions. With the proposed feature, an "augmented" os.File could be
used instead, saving a lot of memory and time for large files.

Although it would be possible to modify the go-github package to get a size (somehow)
for the data in the io.Reader and then set the Content-Length itself, by providing this
functionality in the standard library, then all packages which wrap the http package
could benefit.
@cookieo9
Copy link
Contributor Author

cookieo9 commented Nov 8, 2013

Comment 1:

Alternatively, if http.NewRequest understood a few more std library io.Reader types,
such as:
 - io.LimitedReader
 - io.SectionReader
 - os.File
Then it wouldn't be necessary for users of the http package to create a new type to
satisfy the new interface. Also the scale of the change conceptually would be smaller.

@nightlyone
Copy link
Contributor

Comment 2:

Another very useful thing to solve this problem would by introducing a "io.SizedReader"
interface, which will be satisfied by every io.Reader which additionally knows it's own
size in bytes.
Whether one will use the Size() method or a new ByteSize() method is open to discussion.
This would also allow decompression readers to report the final size without trying to
be seekable.

@minux
Copy link
Member

minux commented Nov 8, 2013

Comment 3:

i think supporting io.ReadSeeker suffices to cover io.SectionReader and os.File.
Then we only need to add support for io.LimitedReader.
For decompression readers, we can wrap them into a io.LimitedReader.

@bradfitz
Copy link
Contributor

Comment 4:

Not LimitedReader.  That's just a max.
*os.File maybe, but that requires statting and seeking to figure out where you're at.
Likewise with SectionReader, which doesn't require a stat but does require a Seek (but
the seek is guaranteed to succeed).  Also http://play.golang.org/p/Ee5I6oesXW makes me
nervous.
I'm nervous about seeking and statting, because for example: if those return an error,
does NewRequest return an error (unlike Go 1.[012]), or does it silently just not set a
Content-Length (surprising).  Neither answer is good.
Adding a new interface like interface { ContentLength() int64 } would infect or
encourage infection lots of types, so that's out.  Likewise with a hidden interface.
Adding io.SizedReader is close to interesting, but I actually want io.SizeReaderAt (see
http://talks.golang.org/2013/oscon-dl.slide#49) which I've used for a number of things
inside Google.  But even a *
I'm inclined to do nothing.

Status changed to Thinking.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 5:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 6:

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

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 7:

Labels changed: added repo-main.

@heidawei
Copy link

a io.Reader come from bytes.NewBuffer() or bytes.NewReader() is inefficiency when copy to a io.Writer,but the io.Reader come from http req.body(it is a io.Reader object) is efficiency.how to solve this problem??

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@bradfitz bradfitz added the v2 A language change or incompatible library change label May 22, 2018
@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 12, 2018
@bradfitz bradfitz self-assigned this Feb 19, 2019
@bradfitz
Copy link
Contributor

I'm going to fold this into the bug about the new HTTP client: #23707

NewRequest will be changing a bunch anyway.

@golang golang locked and limited conversation to collaborators Feb 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Thinking v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

8 participants