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

Don't automatically send push response headers if disabled #50

Closed
wants to merge 1 commit into from

Conversation

WVmf
Copy link

@WVmf WVmf commented Jul 17, 2017

By default a push stream created via Stream#pushPromise() will
immediately send the response headers (the ones provided or a default one
with only the status code).

This commit allows to conditionally skip sending those headers at
Stream creation by explicitly setting the response property to false.
Thus allowing the headers to be manually sent at a later point via
Stream#respond().

This modification also solves a significant part of my node-spdy issue spdy-http2/node-spdy#323.

The push related part of the documentation of node-spdy, should probably also be updated with something like:

Additionally the response property can also be explicitly set to false to skip sending the response headers when creating the push stream. The headers then need to be sent manually using .respond(<statusCode>, {<headers>}) before pushing any data.

By default a push stream created via `Stream#pushPromise()` will
immediately send the response headers (the ones provided or a default one
with only the status code).

This commit allows to conditionally skip sending those headers at
`Stream` creation by explicitly setting the `response` property to `false`.
Thus allowing the headers to be manually sent at a later point via
`Stream#respond()`.
@WVmf
Copy link
Author

WVmf commented Jul 17, 2017

I forgot to mention that I also included a new (http/2 specific) test for this: 'should not send HEADERS on PUSH_PROMISE if disabled' (which is based on the 'should send HEADERS on PUSH_PROMISE' test).

But I'm not completely sure if the test suffices to test this functionality, as I'm not that acquainted with the code base. For example I'm not sure why the push.resume() is needed in the 'pushPromise' callback on the client side.
And I don't fully understand why the 'should send HEADERS on PUSH_PROMISE' succeeds, as I would think the 'headers' event would get called twice in that case: once with the automatically sent {':status': 200} and then a second time with {'a': 'b'}. Or are the headers not sent in that case until the pushPromise callback has finished, and thus the sendHeaders({ a: 'b' }) get combined with the default one before they are sent out?

@WVmf
Copy link
Author

WVmf commented Aug 17, 2017

Just noticed this would also fix #36.

jacobheun added a commit to jacobheun/spdy-transport that referenced this pull request Nov 2, 2018
jacobheun added a commit to jacobheun/spdy-transport that referenced this pull request Nov 2, 2018
daviddias pushed a commit that referenced this pull request Nov 8, 2018
@jacobheun
Copy link
Contributor

I pulled these changes into the LTS node support fix, #61. It's available in the latest version, 3.0.0. Thanks for submitting this!

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

Successfully merging this pull request may close these issues.

2 participants