Callback implementation and tests for write and end

[whitingj]

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.

[dougwilson]

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]

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).

[whitingj]

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]

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]

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. https://github.com/whitingj/compression/commit/3462f9a031b80334081db406edfed6ebb4cd7785 After implementing it I'm not really sold on that approach.

[dougwilson]

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]

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]

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]

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]

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.

[whitingj]

@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.

[pkehrer]

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]

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