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

Callback implementation and tests for write and end #101

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

whitingj
Copy link

@whitingj whitingj commented Jan 4, 2017

This pull request attempts to resolve #80 and #46. This PR adds conditional support for callbacks in versions of node that support callbacks.

Callbacks will be called on write and end when supported in nodejs (requires nodejs >= 0.12.x).
Credit goes to @jpodwys for the majority of the implementation.

Callbacks will be called on `write` and `end` when supported in nodejs (requires nodejs >= 0.12.x).
Credit goes to @jpodwys for the majority of the implementation.
@dougwilson
Copy link
Contributor

It looks like this has the same issues as other PRs: It will sometimes add callback support to Node.js 0.10 and sometimes not, depending on if the response was compressed. In Node.js 0.10, callbacks should not work at all.

@whitingj
Copy link
Author

whitingj commented Jan 5, 2017

I've updated the PR to address your concern. Node.js 0.10 will never call the callbacks. I also updated the tests to assert that no callbacks were happening (that test failed before I made the additional changes).

index.js Outdated
@@ -35,6 +36,7 @@ module.exports.filter = shouldCompress
*/

var cacheControlNoTransformRegExp = /(?:^|,)\s*?no-transform\s*?(?:,|$)/
var nodeHasCallbacks = (http.OutgoingMessage.prototype.write.length === 3 && http.OutgoingMessage.prototype.end.length === 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @whitingj . Looking at the changes, I feel like someone else did the exact same change and it is problematic. I found my response to that, if you can take it into consideration: #80 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that comment was in response to the following change: 0f617c7 which is basically identical to your change here.

HISTORY.md Outdated
@@ -1,6 +1,9 @@
unreleased
==========

1.7.0 / 2017-01-04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove version bumps from the PR.

package.json Outdated
@@ -1,7 +1,7 @@
{
"name": "compression",
"description": "Node.js compression middleware",
"version": "1.6.2",
"version": "1.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove version bumps from the PR.

@dougwilson dougwilson self-assigned this Jan 5, 2017
@whitingj
Copy link
Author

whitingj commented Jan 5, 2017

I've read through your comment. I've have been thinking a lot about it and doing some research.

The good news is that both spdy (https://github.com/indutny/node-spdy#api) and http2 (https://github.com/molnarg/node-http2/blob/master/lib/http.js#L1092) use http.OutgoingMessage in their prototype. So doing feature detection on http.OutgoingMessage.prototype.write.length seems completely appropriate and inline Express.js TC goal to support spdy and http2.

... so bugs from using this module with others causing almost impossible to debug hangs is not something I am looking forward to answering for a long time to come :)

I can see only 2 ways to solve this problem.

  1. Throw an error if the callee supplies a callback and the underlying request doesn't support it.
  2. Honor the OutgoingMessage api and pass through the callbacks in versions of node that support it.

The first approach eliminates your concern about "impossible to debug hangs" but will break anyone who is currently supplying a callback. The second approach puts the onus on the developer to only use middleware that support callbacks (if he needs the functionality) but can lead to "hangs" if any of the middleware he is using doesn't support the callback. @dougwilson do you see any other approaches?

IMHO i think the second approach is the right one. It is best practice when proxying or monkey patching to maintain the original interface. So when compression overwrites OutgoingMessage.write or OutgoingMessage.end it should maintain the same function signature. I realize other middleware may not honor the callback but compression can be better and maintain the full functionality.

@dougwilson what is your preference for implementation?

@dougwilson
Copy link
Contributor

So doing feature detection on http.OutgoingMessage.prototype.write.length seems completely appropriate and inline Express.js TC goal to support spdy and http2 .

No, it is not. The whole point is that we should not imply that that is going to be in the prototype. I'm not sure if the http2 module has changed since my comment, but that was simply an example, not the rule. If you distill it to a rule, the rule should be like you stated:

it is best practice when proxying or monkey patching to maintain the original interface.

That means that if you are checking an object completely unrelated to the object you are given, how can you even be sure that you are maintaining the original interface at all?

@whitingj
Copy link
Author

whitingj commented Jan 5, 2017

I've removed the version bump from the PR.

I also implemented the first approach of throwing an error in a separate branch in my repo to see what it would be like.
whitingj@3462f9a
After implementing it I'm not really sold on that approach.

@dougwilson
Copy link
Contributor

Also, I don't think you understood that http2 source code; the OutgoingMessage on the line you linked to is not the one from Node.Js, rather it refers to the object defined in that file directly. AFAICT http2 module does not derrive from the Node.js OutgoingMessage instance at all

@whitingj
Copy link
Author

whitingj commented Jan 5, 2017

You're right. When I read the code I didn't realize OutgoingMessage was defined in the file and not coming from http.OutgoingMessage. Regardless they are attempting to maintain the same API as the https and http modules. https://github.com/molnarg/node-http2/blob/master/lib/http.js#L4

No, it is not. The whole point is that we should not imply that that is going to be in the prototype. I'm not sure if the http2 module has changed since my comment, but that was simply an example, not the rule.

How would you like the feature detection to be done? It is easy enough to check with req.write.length rather than http.OutgoingMessage.prototype.write.length and I can do that if you prefer.

@dougwilson
Copy link
Contributor

That sounds OK on the surface, but I suspect that will eventually run into issues because I'm sure there are a lot of monkey patching that does something like res.write = function () { return write.apply(this, arguments); } or similar where the .length may not reflect what it can do since it will eventually call .apply with the original arguments. If I knew a solution, I'm sure it would have been fixed already :)

@dougwilson
Copy link
Contributor

dougwilson commented Jan 6, 2017

And to be clear about what we want to do: we have determined over the years we need to make as little assumptions, especially with all the mocks and other servers out there. Even things like FastCGI implemented in Node.js and hosted with this on it can end up without the core Node.js prototype on the chain.

The more I think about your comments, I think we could simply defer fixing this until Express 5.0 and issuing a breaking change in this module that (1) requires Node.js 0.12+ (or even 4+) and changes the assumption that res.write and other methods accept a callback instead of even trying to detect it, which is basically a loosing battle in JavaScript.

So my proposal: let's just make the PR assume that everything supports callbacks and defer landing this change until a major version around the Express 5.0 time frame (which I think would be a few month or so).

@whitingj
Copy link
Author

whitingj commented Jan 6, 2017

Your approach sounds reasonable. I understand being conservative with changes as so many different apps use this module. I'll update the PR to assume that there is callback support for write and end. I agree with the major version bump and would require at a minimum node 0.12+ so compression doesn't have to worry about the callback mismatch between zlib and OutgoingMessage in older versions.

FWIW my personal preference has always been to just assume that callbacks are supported and pass them through. But node <= 0.10 made that tricky.

Admittedly waiting until expression 5.0 releases it is a little disappointing as I would like to see this change land sooner rather than later. The irony here is that my app was hanging due to compression not supporting callbacks.

var hasCallbacks = false
var callbackOutput = []
var server = createServer(null, function (req, res) {
// hasCallback check can be removed once this module only supports node >= 0.12 and .travis.yml is updated to test on node >= 0.12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the plan, I would just go ahead and remove this testing conditional and remove the unsupported versions from the travis file right in this PR :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@whitingj
Copy link
Author

whitingj commented Jan 6, 2017

@dougwilson thanks for being so responsive on this thread. I've updated the PR to just pass through the callbacks. I didn't update the node version in package.json as I figured you'd do that when you merge and update to a new major version. Please review the changes and let me know if I need to change anything. Thanks and I'll be bugging you again closer to the release of express 5.0.

@dougwilson dougwilson modified the milestone: 2.0 Jan 6, 2017
@pkehrer
Copy link

pkehrer commented May 3, 2020

Hey y'all, any chance we want to revive this? I'm running into an issue where I can't properly handle errors while attempting to stream responses using gzip compression. Errors from res.write calls end up unhandled without the res.write callback. Do you need some help getting this PR cleaned up to be merged, or are there outstanding issues with it? Thanks @dougwilson for your continued support of this project!

@dougwilson
Copy link
Contributor

Looks like it just needs a rebase now. I will rebase it and get it pushed out.

@@ -1,6 +1,7 @@
unreleased
==========

* Callbacks available in `write` and `end` when supported in nodejs (requires nodejs >= 0.12.x)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the callback argument was added in 0.11.6, actually!

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

Successfully merging this pull request may close these issues.

4 participants