spdy-http2 / node-spdy

SPDY server on Node.js
2.81k stars 196 forks source link

Expose CONNECT requests via the 'connect' event #157

Closed ploer closed 10 years ago

ploer commented 10 years ago

The node http interface exposes CONNECT requests via a 'connect' event, but in recent releases of node-spdy I am seeing CONNECT requests being exposed via the 'request' event like other requests.

The desired behavior existed in earlier versions of node-spdy (see issue #49), but it appears to me that it regressed with the changes in version 1.15.

indutny commented 10 years ago

Generally appears to be looking good, but could you please provide a test for this change? Thanks for finding it!

ploer commented 10 years ago

Applied the changes you suggested and added a test.

Regarding your comment on client-side behavior, my impression was that the start() function was just used on the server side, is that mistaken?

I took a look at what it would take to add support for client-side CONNECT handling, which also seems to be missing, but I haven't been able to work it out yet.

ploer commented 10 years ago

I wasn't too keen on the one-sidedness of that self-test, so I finished getting the client-side CONNECT handling working well enough to at least complete a round-trip test along the lines of the example CONNECT code in the node http docs.

Unfortunately it looks like it's failing tests on 0.11 right now, I'll take a look at figuring that out tomorrow.

In general, I'm not as comfortable with the client-side changes as with the server changes, but they shouldn't break anything unless someone is trying to actually use them. Would you be interested in my pursuing these changes or should I go ahead and revert back to server-side only?

ploer commented 10 years ago

Updated to pass tests on 0.11.

ploer commented 10 years ago

Made the changes you suggested.

The bit with using the _events internal was a misguided attempt to remove only the listener attached from the http library, and leave any listener that might have been attached by the consumer of the library. Obviously, what I was doing wasn't going to work, and I don't know of any good way to do that, so I switched back to a removeAllListeners. I think this should be fine, since the usual pattern would be to attach to 'data' only after the connect event.

The other thing going on in that clause is that I am using it to set skipBodyParsing only in 0.11+ (since the http lib only adds a 'data' listener in 0.11+). The body parsing has useful side effects in 0.8 and 0.10, but is very different, causes problems, and is unneeded in 0.11.

So that clause could alternatively be in a !this.onData, which seems to be used elsewhere as a sort of 'is this 0.11' check. Or, we could try to not rely on body parsing in earlier versions, but I suspect that would lead to a larger change set.

indutny commented 10 years ago

Review time! ;)

indutny commented 10 years ago

Landed in c183ea0, thank you!

ploer commented 10 years ago

Thank you, much appreciated!