avoid arbitrary memory use in upload #55

Merged
merged 1 commit into from Sep 4, 2017

Conversation

Projects
None yet
4 participants
Member

rogpeppe commented Aug 23, 2017

When uploading a file, the code was reading the entire
body into memory before making the request.
This caused out-of-memory errors when
uploading large files.

Now, if you provide a ReadSeeker, it will seek to
the start of the file rather than reading the whole
thing into memory.

It's tempting to change the API so that it requires
a ReadSeeker rather than a Reader, but that would break
the goose.v2 compatibility guarantee.

axw approved these changes Aug 29, 2017

http/client.go
}
+ req.Body = body
@axw

axw Aug 29, 2017

Member

please add a comment stating why you're setting req.Body, rather than passing it into NewRequest

@rogpeppe

rogpeppe Aug 29, 2017

Member

Good catch. It turns out that it wasn't necessary in fact, as NewRequest does a dynamic type check for io.ReadCloser.

I've also added some more tests that check the new logic - there were some significant gaps in coverage previously.

avoid arbitrary memory use in upload
When uploading a file, the code was reading the entire
body into memory before making the request.
This caused out-of-memory errors when
uploading large files.

Now, if you provide a ReadSeeker, it will seek to
the start of the file rather than reading the whole
thing into memory.

It's tempting to change the API so that it requires
a ReadSeeker rather than a Reader, but that would break
the goose.v2 compatibility guarantee.
Member

rogpeppe commented Aug 29, 2017

$$merge$$

Member

mhilton commented Aug 30, 2017

$$merge$$

@urosj urosj merged commit 4554160 into go-goose:v2 Sep 4, 2017

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