Callback implementation and tests for write and end #101

whitingj · 2017-01-04T19:39:14Z

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.

@jpodwys

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 · 2017-01-04T19:48:52Z

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.

…o assert that no callbacks are made for nodejs < 0.12.x

whitingj · 2017-01-05T01:13:55Z

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

dougwilson · 2017-01-05T02:59:42Z

index.js

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


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)

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

dougwilson · 2017-01-05T03:00:06Z

HISTORY.md

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

+1.7.0 / 2017-01-04


Please remove version bumps from the PR.

dougwilson · 2017-01-05T03:00:12Z

package.json

@@ -1,7 +1,7 @@
 {
  "name": "compression",
  "description": "Node.js compression middleware",
-  "version": "1.6.2",
+  "version": "1.7.0",


Please remove version bumps from the PR.

whitingj · 2017-01-05T23:18:18Z

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.

Throw an error if the callee supplies a callback and the underlying request doesn't support it.
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 · 2017-01-05T23:26:46Z

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 · 2017-01-05T23:30:48Z

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 · 2017-01-05T23:31:56Z

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 · 2017-01-05T23:40:50Z

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 · 2017-01-05T23:58:29Z

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 · 2017-01-06T00:02:16Z

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 · 2017-01-06T00:31:36Z

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.

…acks are always supported.

dougwilson · 2017-01-06T00:40:54Z

test/compression.js

+      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


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

whitingj · 2017-01-06T00:44:47Z

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

…rsions of node from .travis.yml.

pkehrer · 2020-05-03T12:22:54Z

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 · 2020-05-03T15:04:06Z

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

theneva · 2020-05-27T11:34:56Z

HISTORY.md

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

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


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

Callback implementation and tests for write and end.

a68265e

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.

whitingj added 2 commits January 4, 2017 18:07

Callbacks are only used if nodejs supports callbacks. Updated tests t…

a7c7615

…o assert that no callbacks are made for nodejs < 0.12.x

Fixing lint error, missing space after “//“

ce0f02f

dougwilson suggested changes Jan 5, 2017

View reviewed changes

dougwilson self-assigned this Jan 5, 2017

dougwilson added bug pr labels Jan 5, 2017

Removing the version bump.

9d55cfd

Removing the feature detection checks. This PR now assumes that callb…

4a8e87f

…acks are always supported.

dougwilson reviewed Jan 6, 2017

View reviewed changes

dougwilson modified the milestone: 2.0 Jan 6, 2017

Removing conditional checks in unit tests and removing unsupported ve…

2fdf90f

…rsions of node from .travis.yml.

dougwilson added the needs rebase label Jan 15, 2018

dougwilson force-pushed the master branch 3 times, most recently from d7bb81b to cd957aa Compare May 30, 2018 04:09

This was referenced Sep 25, 2019

Heroku deployment telegraf/telegraf#610

Closed

Fix answering to webhook query in express app with compression middleware telegraf/telegraf#756

Closed

jonchurch mentioned this pull request May 27, 2020

Proxy res.end(callback) calls correctly expressjs/session#751

Open

theneva reviewed May 27, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Callback implementation and tests for write and end #101

Callback implementation and tests for write and end #101

whitingj commented Jan 4, 2017

dougwilson commented Jan 4, 2017

whitingj commented Jan 5, 2017

dougwilson Jan 5, 2017

dougwilson Jan 5, 2017

dougwilson Jan 5, 2017

dougwilson Jan 5, 2017

whitingj commented Jan 5, 2017 •

edited

dougwilson commented Jan 5, 2017

whitingj commented Jan 5, 2017

dougwilson commented Jan 5, 2017

whitingj commented Jan 5, 2017

dougwilson commented Jan 5, 2017

dougwilson commented Jan 6, 2017 •

edited

whitingj commented Jan 6, 2017

dougwilson Jan 6, 2017

whitingj Jan 6, 2017

whitingj commented Jan 6, 2017

pkehrer commented May 3, 2020

dougwilson commented May 3, 2020

theneva May 27, 2020

Callback implementation and tests for write and end #101

Are you sure you want to change the base?

Callback implementation and tests for write and end #101

Conversation

whitingj commented Jan 4, 2017

dougwilson commented Jan 4, 2017

whitingj commented Jan 5, 2017

dougwilson Jan 5, 2017

Choose a reason for hiding this comment

dougwilson Jan 5, 2017

Choose a reason for hiding this comment

dougwilson Jan 5, 2017

Choose a reason for hiding this comment

dougwilson Jan 5, 2017

Choose a reason for hiding this comment

whitingj commented Jan 5, 2017 • edited

dougwilson commented Jan 5, 2017

whitingj commented Jan 5, 2017

dougwilson commented Jan 5, 2017

whitingj commented Jan 5, 2017

dougwilson commented Jan 5, 2017

dougwilson commented Jan 6, 2017 • edited

whitingj commented Jan 6, 2017

dougwilson Jan 6, 2017

Choose a reason for hiding this comment

whitingj Jan 6, 2017

Choose a reason for hiding this comment

whitingj commented Jan 6, 2017

pkehrer commented May 3, 2020

dougwilson commented May 3, 2020

theneva May 27, 2020

Choose a reason for hiding this comment

whitingj commented Jan 5, 2017 •

edited

dougwilson commented Jan 6, 2017 •

edited