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

connect-injector does not work with node 0.11 and express.compress() #7

Closed
wrr opened this issue Dec 19, 2013 · 2 comments · Fixed by #8
Closed

connect-injector does not work with node 0.11 and express.compress() #7

wrr opened this issue Dec 19, 2013 · 2 comments · Fixed by #8

Comments

@wrr
Copy link
Contributor

wrr commented Dec 19, 2013

Unit tests that access modified response body fail (the body is missing). It can be reproduced with:

nvm use 0.11
npm test

I identified the error to be in this line https://github.com/daffl/connect-injector/blob/master/lib/connect-injector.js#L151.
The line assumes that a call to super(data), will not invoke write(data). This assumption was true in node 0.10 end(data) outputs data directly to an outgoing connection (https://github.com/joyent/node/blob/v0.10/lib/http.js#L959). But with node 0.11 end(data), delegates this job to write(data) (https://github.com/joyent/node/blob/master/lib/_http_outgoing.js#L524).

This causes injector's mixed in write(data) to be invoked again, and the data is buffered and never passed to the original handler.

A fix would be to replace the call to super(data) with original_handler.write(data), followed by super() (with no data passed). I tried an ugly way of saving the original write function:

var real_write = res.write.bind(res);

and this fixed the tests. I'm not sure though that this is fully correct, maybe there is a nicer way that allows somehow to refer to write._super() from end()?

wrr added a commit to wrr/connect-injector that referenced this issue Dec 19, 2013
@wrr
Copy link
Contributor Author

wrr commented Dec 19, 2013

For a reference I'm attaching my fix, but it is not very elegant. I'm not familiar with uberproto, it may allow a much nicer way to fix the problem.

wrr added a commit to wrr/connect-wwwhisper that referenced this issue Dec 20, 2013
@wrr
Copy link
Contributor Author

wrr commented Feb 3, 2014

The same problem happens with node 0.10 when compress middleware is used (higher in the stack than the connect-injector, so this is a different issue from #4). The small breaking example is:

var express = require('express');
var app = express();

// XXX
app.use(express.compress());

var injector = require('connect-injector')(
  function when(req, res) {
    return true;
  }, function converter(callback, content, req, res) {
    callback(null, content + ' world');
  });

app.use(injector);

app.get('/', function(req, res) {
  res.send('Hello');
});

var port = process.env.PORT || 5000;
app.listen(port, function() {
  console.log('Listening on ' + port);
});

This returns an empty response if the line marked 'XXX' is enabled.

The problem is that the compress middleware also invokes write() from the end() (please see). The patch included above also solves this problem.

Broken compress middleware support is quite urgent in my case, let me know if I can do anything to help fixing this.

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 a pull request may close this issue.

1 participant