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

stream_body uses response.body = iostream #421

Merged
merged 1 commit into from Aug 7, 2017

Conversation

Projects
None yet
3 participants
@jrochkind
Contributor

jrochkind commented Aug 3, 2017

instead of a loop of response.stream.write's.

Rails still streams with this new implementation. But it is delivering the HTTP response
headers a lot faster this way, almost immediately.

With the previous way, with a 1G original file accessed via
/downloads, I was seeing an 11 second wait even to get HTTP headers.

Not sure why, it could be because it takes that long to even get
the first byte from fedora, and it's not sending anything until it does?
But I tried to mess around with things using the response.stream.write
API, and was not able to improve things.

Browsers are willing to wait that long, but it's not a great UX -- you get
a 'waiting for connection' popup in chrome until the HTTP headers (in my
case 11 seconds later), only at that point does it show up as a download in progress.

But some non-browser user agents are not even willing to wait that long. imgix.com
is willing to wait at most 10 seconds for HTTP headers (after that it will wait quite a bit longer for the entire content to stream), which honestly seems like it should be pretty generous.
Other CDNs may be similar.

Ensuring quick HTTP header response seems a good idea, and this change
seems to accomplish it. Even my 1G file had nearly immediately HTTTP header
delivery, although of course you still have to wait for the actual file contents
to download.

For some background on response.body = stream as a valid rails
streaming API, see rails/rails#18714

jrochkind added a commit to sciencehistory/chf-sufia that referenced this pull request Aug 3, 2017

@jcoyne

This comment has been minimized.

Show comment
Hide comment
@jcoyne

jcoyne Aug 3, 2017

Member

Looks good to me, but let's have one more person have a look at it. This has been a perpetual area of consternation for me.

Member

jcoyne commented Aug 3, 2017

Looks good to me, but let's have one more person have a look at it. This has been a perpetual area of consternation for me.

@jcoyne

@cbeer and I reviewed this together.

stream_body uses response.body = iostream
instead of a loop of response.stream.write's.

Rails still streams like this. But it is delivering the HTTP response
headers a lot faster this way, almost immediately.

With the previous way, with a 1G original file accessed via
/downloads, I was seeing an 11 second wait even to get HTTP headers.

Not sure why, it could be because it takes that long to even get
the first byte from fedora, and it's not sending anything until it does?
But I tried to mess around with things using the response.stream.write
API, and was not able to improve things.

Browsers are willing to wait that long, but it's not a great UX -- you get
a 'waiting for connection' poup in chrome until those original headers, at
which point it shows up as a download in progress.

But some non-browser user agents are not willing to wait that long. imgix.com
is willing to wait at most 10 seconds for HTTP headeres (after that it will wait quite a bit longer for the entire content to stream). Other CDNs may be similar.

Ensuring quick HTTP header response seems a good idea, and this change
seems to accomplish it. Even my 1G file had nearly immediately HTTTP header
delivery, although of course you still have to wait for the file.

For some background on response.body = stream as a valid rails
streaming API, see rails/rails#18714
@jcoyne

jcoyne approved these changes Aug 3, 2017

ensure
response.stream.close
render nothing: true

This comment has been minimized.

@cam156

cam156 Aug 4, 2017

Contributor

I wonder if this should be render :head (see http://guides.rubyonrails.org/layouts_and_rendering.html#using-head-to-build-header-only-responses) It would be more explicit you are putting out headers

@cam156

cam156 Aug 4, 2017

Contributor

I wonder if this should be render :head (see http://guides.rubyonrails.org/layouts_and_rendering.html#using-head-to-build-header-only-responses) It would be more explicit you are putting out headers

This comment has been minimized.

@jrochkind

jrochkind Aug 7, 2017

Contributor

I'm not totally sure what it does, I copied it from the example at rails/rails#18714 (comment), and then confirmed it worked properly -- with our entire code here, the headers we set are sent, the body we set is sent, in a streaming fashion.

I think it tells Rails not to render with it's normal rendering stuff, we're going to set response.body manually ourselves, along with response.headers manually ourselves. We don't want render :head, we are sending a body, not a header-only response, we're just setting the body manually to a streaming IO object with response.body =, instead of using on rails default rendering pipeline, in order to get streaming.

@jrochkind

jrochkind Aug 7, 2017

Contributor

I'm not totally sure what it does, I copied it from the example at rails/rails#18714 (comment), and then confirmed it worked properly -- with our entire code here, the headers we set are sent, the body we set is sent, in a streaming fashion.

I think it tells Rails not to render with it's normal rendering stuff, we're going to set response.body manually ourselves, along with response.headers manually ourselves. We don't want render :head, we are sending a body, not a header-only response, we're just setting the body manually to a streaming IO object with response.body =, instead of using on rails default rendering pipeline, in order to get streaming.

@jrochkind jrochkind merged commit c7fdaa7 into master Aug 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

jrochkind added a commit to sciencehistory/chf-sufia that referenced this pull request Aug 7, 2017

jcoyne added a commit that referenced this pull request Nov 22, 2017

Revert part of #421
Because `render nothing:true` is not supported in Rails 5.1

jcoyne added a commit that referenced this pull request Nov 22, 2017

Revert part of #421
Because `render nothing:true` is not supported in Rails 5.1

jcoyne added a commit that referenced this pull request Dec 21, 2017

botimer added a commit to mlibrary/hydra-head that referenced this pull request Apr 4, 2018

Fix download streaming with self.response_body=
We have been seeing lots of extra memory usage when downloading large
files from Fedora. There was a version of this fix applied in samvera#421, but
it was reverted in samvera#427. This uses the recommended technique of setting
the controller's response_body to the stream.

In looking through the ActionController source, this should both start
the output (sending headers), and flag the render as having performed,
so implicit template resolution/rendering will be skipped.

botimer added a commit to mlibrary/hydra-head that referenced this pull request Apr 4, 2018

Fix download streaming with self.response_body=
We have been seeing lots of extra memory usage when downloading large
files from Fedora. There was a version of this fix applied in samvera#421, but
it was reverted in samvera#427. This uses the recommended technique of setting
the controller's response_body to the stream.

In looking through the ActionController source, this should both start
the output (sending headers), and flag the render as having performed,
so implicit template resolution/rendering will be skipped.

@cjcolvar cjcolvar deleted the stream_body_use_body_equals branch Apr 5, 2018

cjcolvar added a commit that referenced this pull request Apr 5, 2018

Revert part of #421
Because `render nothing:true` is not supported in Rails 5.1

cjcolvar added a commit that referenced this pull request Apr 5, 2018

Fix download streaming with self.response_body=
We have been seeing lots of extra memory usage when downloading large
files from Fedora. There was a version of this fix applied in #421, but
it was reverted in #427. This uses the recommended technique of setting
the controller's response_body to the stream.

In looking through the ActionController source, this should both start
the output (sending headers), and flag the render as having performed,
so implicit template resolution/rendering will be skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment