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

Fix stall when piping gzipped response #1568

Merged
merged 1 commit into from May 8, 2015

Conversation

kevinoid
Copy link
Contributor

@kevinoid kevinoid commented May 8, 2015

When the response content is piped through additional streams for decoding (e.g. for gzip decompression), pause and resume calls should be propagated to the last stream in the pipeline so that back pressure propagates correctly.

This avoids an issue where simultaneous back pressure from the content decoding stream and from a stream to which Request is piped could cause the response stream to get stuck waiting for a drain event on the content decoding stream which never occurs. See #1567 for an example.

This commit also renames dataStream to responseContent to remedy my previous poor choice of name, since the name will be exposed on the Request instance it should be clearer and closer to the name used to refer to this data in the relevant RFCs.

Fixes #1567

When the response content is piped through additional streams for
decoding (e.g. for gzip decompression), pause and resume calls should be
propagated to the last stream in the pipeline so that back pressure
propagates correctly.

This avoids an issue where simultaneous back pressure from the content
decoding stream and from a stream to which Request is piped could cause
the response stream to get stuck waiting for a drain event on the
content decoding stream which never occurs.  See request#1567 for an example.

This commit also renames dataStream to responseContent to remedy my
previous poor choice of name, since the name will be exposed on the
Request instance it should be clearer and closer to the name used to
refer to this data in the relevant RFCs.

Fixes request#1567

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
simov added a commit that referenced this pull request May 8, 2015
Fix stall when piping gzipped response
@simov simov merged commit 6e20a9f into request:master May 8, 2015
@simov
Copy link
Member

simov commented May 8, 2015

@kevinoid 👍

@kevinoid
Copy link
Contributor Author

kevinoid commented May 8, 2015

Thanks @simov!

kevinoid added a commit to kevinoid/request that referenced this pull request May 8, 2015
Due to request#1568, we now propagate pause and resume to the response content
stream, rather than the response stream.  However, when pause is called
before the response arrives, the pause is still being applied to the
response object directly.  Fix this by applying it to the response
content stream in both cases.  This avoids the issue that if pause is
called before the response arrives, it can not be resumed.

Also add tests of the pause/resume behavior for both the gzip and
non-gzip case both before and after the response has arrived.

This commit also makes the ancillary change that resume is now called
unconditionally (when defined) in Redirect, since we always want to dump
the response data.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/request that referenced this pull request May 8, 2015
Due to request#1568, we now propagate pause and resume to the response content
stream, rather than the response stream.  However, when pause is called
before the response arrives, the pause is still being applied to the
response object directly.  Fix this by applying it to the response
content stream in both cases.  This avoids the issue that if pause is
called on a gzip request before the response arrives, it pauses the
response then resumes the response content, meaning the response can not
be resumed.

Also add tests of the pause/resume behavior for both the gzip and
non-gzip case both before and after the response has arrived.

This commit also makes the ancillary change that resume is now called
unconditionally (when defined) in Redirect, since we always want to
dump the response data (and it was previously called unconditionally in
onRequestResponse).

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid added a commit to kevinoid/request that referenced this pull request May 8, 2015
Due to request#1568, we now propagate pause and resume to the response content
stream, rather than the response stream.  However, when pause is called
before the response arrives, the pause is still being applied to the
response object directly.  Fix this by applying it to the response
content stream in both cases.  This avoids the issue that if pause is
called on a gzip request before the response arrives, it pauses the
response then resumes the response content, meaning the response can not
be resumed.

Also add tests of the pause/resume behavior for both the gzip and
non-gzip case both before and after the response has arrived.

This commit also makes the ancillary change that resume is now called
unconditionally (when defined) in Redirect, since we always want to
dump the response data (and it was previously called unconditionally in
onRequestResponse).

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird issue when piping a chunked data from HTTP to a file in OS X
2 participants