Implementation

[Swaagie]

While working on https://github.com/flatiron/union I noticed that on-headers is used by alot of the middleware. It's breaking the compatibility of flatiron with the latest connect/express middlware. The main issue is: cookies are not written because of writeHead not being called. This problem is not my concern though as union existed to patch against those in the first place. This idea behind this issue is to create an open discussion about the current pattern. I'm unsure I understand the full implications and ideas. Feel free to call me out wherever ;)

1. Each middleware layer that uses on-headers and that is implemented will consecutively overwrite writeHead. Correct me if I'm wrong but it seems to make the call chain length == number of middleware modules used. A possible alternative would be to use a full EventEmitter pattern that is exposed as singleton on the on-headers module. Not only would other modules be able to hook into on-headers (by emitting specific events) it should also prevent multiple writeHead overwrites.
2. Union deliberately hooked into the _renderHeaders so that parts of the writeHead function would not have to be rewritten. The setWriteHeadHeaders comment suggest its redoing parts of the core, would it be possible to hook in the lower level functions of the stream?
3. Several emits in node core could also be suggested, emitting specific events from writeHead that can be listened to, similar to the close/timeout.

[Swaagie]

For reference, senchalabs/connect@b8292a4 would be the offending change

[dougwilson]

The issue is with union, not this module. I see you are emitting a "header"event in it: https://github.com/flatiron/union/blob/921552e45c13b4bb9fbb717bdc0aba0d0ce1f756/lib/response-stream.js#L62

This is not Node.js core behavior. This module is designed to work with Node.js core OutgoingMessage objects only. union would need to work with this. _renderHeaders is not a publish method, and we don't make a habit of touching or overriding non-public things in Node.js core.

Each middleware layer that uses on-headers and that is implemented will consecutively overwrite writeHead. Correct me if I'm wrong but it seems to make the call chain length == number of middleware modules used. A possible alternative would be to use a full EventEmitter pattern that is exposed as singleton on the on-headers module. Not only would other modules be able to hook into on-headers (by emitting specific events) it should also prevent multiple writeHead overwrites.

This is independent of the issue you are describing and I can easily do it if it becomes an issue; we already do this for finished.

Union deliberately hooked into the _renderHeaders so that parts of the writeHead function would not have to be rewritten.

Also, _renderHeaders is not even a reliable way to emit that event, because there is a path where _renderHeaders is never called in the entire pipeline, and thus modules would not be able to set headers at the end of the request: https://github.com/joyent/node/blob/v0.10.24/lib/http.js#L1176

Several emits in node core could also be suggested, emitting specific events from writeHead that can be listened to, similar to the close/timeout.

That would be nice if you can get Node.js core to accept this. I believe it has been turned down in the past.

For reference, senchalabs/connect@b8292a4 would be the offending change

I can investigate possibly adding a secondary hook into _renderHeaders, but I have to figure out the implications first. This module provides two guarantees with the call that needs to be kept: all headers, even ones passed to res.writeHead are accessible using res.getHerader in your callback and and sync modifications in the callback will be sent to the client.

[Swaagie]

I agree with you union is to blame, also union has been around from the start of connect middleware. Theoretically union should be phased out entirely. No more patching and general usability for middleware should result in flatiron working with the middleware without union. I'll discuss if we perhaps need to rework the http plugin of flatiron. Or co-develop to make sure middleware is as wide applicable as possible.

This is not Node.js core behavior. This module is designed to work with Node.js core OutgoingMessage objects only. union would need to work with this. _renderHeaders is not a publish method, and we don't make a habit of touching or overriding non-public things in Node.js core.

I agree, however I personally don't see a difference in overriding a 'public' or 'private' method of node.js, since the distinction is arbitrary at best. But I do understand why you opted to overwrite writeHead and its a valid reason.

This is independent of the issue you are describing and I can easily do it if it becomes an issue; we already do this for finished.

Unsure how big the performance gains would be, benchmarks should prove if in fact this is a performance loss/gain. I'll see if I can do some benchmarking (need to find the time) by using the same pattern as finished. The large amount of frameworks depending on the middleware makes any performance gain significant.

That would be nice if you can get Node.js core to accept this. I believe it has been turned down in the past.

Yeah I recall something like that as well, i'll see if I can dug up the issue and pull some strings here and there.

[dougwilson]

Hi @Swaagie do you have any more thoughts on this?