onFinished(res) may trigger early

[dougwilson]

So apparently there was a false assumption on the already-finished detection of OutgoingMessage that the socket property will always be populated. From https://github.com/joyent/node/blob/v0.10.32/lib/http.js#L455 in 0.10 (0.8 is similar), socket is not assigned to OutgoingMessage until a later time in the cycle. I believe this occurs with pipelined requests, from reading the Node.js source code, which would explain why it's rarely seen (most things have pipelining turned off and doing things like putting Node.js behind nginx will prevent pipelining from reaching Node.js).

[dougwilson]

Funny enough, the line at https://github.com/joyent/node/blob/v0.10.32/lib/http.js#L1116 was not added until 0.10.0, so it means all versions before that (0.9.x and 0.8.x) are subtly broken -____-

[Fishrock123]

You mean those versions just don't act correctly in the first place?

[Fishrock123]

Related commit: nodejs/node-v0.x-archive@e2400f8

Related issue: nodejs/node-v0.x-archive#4967

Well, that's kinda how fixes work sometimes. :|
We should probably deprecate support 0.8 soon anyways.

[dougwilson]

Right. It seems that it was decided a bug was not to be fixed until the next feature version, but that feature version was even released. I could see if it was after 0.10 was released, but whatever. Node.js doesn't always make the best support decisions, lol

[dougwilson]

Anyway, I could monkey-patch ServerResponse.prototype.assignSocket, though I don't particularly want to, since it's an extra thing to slow things down. Otherwise ee-first module needs the ability to remove the group of event listeners.

[Fishrock123]

@dougwilson is it that this module is broken on those, or just the edge case that this fixes?

[dougwilson]

This module is 100% broken when pipelined requests come in faster than they are handled. But as far as monkey-patching, I really mean I don't want to unless there is a feature-detection I can use to restrict it only to 0.8

[Fishrock123]

Like, does the second part affect the module as a whole, or is it just that this issue cannot be fixed on 0.8?

If it's the latter, I think poking people to upgrade if they happen to encounter it is a better option.

[Fishrock123]

@dougwilson though it looks like you can the node version in-code via process.version.

[dougwilson]

I already pushed the version that works perfectly on 0.8, it's just lame. I noticed something I can use to feature detect that it'll emit the socket event.

though it looks like you can the node version in-code via process.version.

Yea, and I make web sites that look in User-Agent :) But no, I don't because it's too easy for that to break unless you pull in a whole proper library like semver.