-
Notifications
You must be signed in to change notification settings - Fork 7.3k
More useful web socket upgrade API #4813
Comments
For simplicity, |
This change would really simplify things and would greatly reduce the surface area that websocket libraries would need to touch such that you could just pass them a
|
This seems good to me, except that:
I'm thinking primarily of the case of debugging a long chain of Restify handlers, where we may (unintentionally) try to use a connection for both kinds of operation. |
Meh. Sure, why not. We could also already have stashed it on req.socket and res.connection and everywhere else you can imagine. And yes, upgrading after writing/etc, or writing/etc after upgrading, shall emit an error event. |
@isaacs: Please don't take this as a complaint or anything negative, I'm genuinely interested but am having difficulty understanding how your idea works.
By 'automatically upgrade', does this mean just treating HTTP requests with the Upgrade header as regular responses with a special flag instead of a separate event, or something else? If the former, Legacy code will not know the difference between regular requests and upgrades, and may accidentally attempt to use the
I'm a bit confused as to how this will work either, I was under the impression that under your proposal, Or does the 101 status code need to be manually sent with |
I very much like this proposal and second @jclulow's suggestion of not adding a third arg to the request handler. This would make it easier to continue using connect style code that already has it's own third parameter. I don't really care if it's returned from upgrade or available as a property on req. |
This is a great change. We underwent a lot of pain in engine.io to make sure we don't claim upgrade requests that other handlers might be expecting (since we only want to handle the ones that have a certain More information of our approach here: @shtylman who worked with me on that patch might have other comments about this API change. |
I also don't see the need for the socket in the event signature considering that pretty much everyone is used to |
How would this be BC? Maybe if |
This is pretty much what I tried to accomplish with #3036, but I suppose I never worded it quite right. (Thanks for referencing, @adammw.) Biggest issue with #3036 was, if I understood correctly, backwards compatibility in the API, which is a bit difficult to accomplish in the API proposal I made. But now regarding this issue... As @adammw already pointed out, there's the subtlety of what exactly
I'm also wondering if |
On 19 March 2013 04:50, Adam Malcontenti-Wilson
It would be good to be able to use the regular Response machinery to Perhaps you would call res.upgrade() in place of res.end(), which |
I am +1 on not upgrading by default and allowing the user to handle that. I am -1 on the third parameter (as noted by some others). I think As for responding with headers. I don't see why |
With regards to backwards compat. I don't really see any good automatic Unlike a previous upgrade-ex proposal/module, reusing the request (while it But anyhow, please discuss any other bc ideas I may have missed. Adam Malcontenti-Wilson |
How bad of a backwards compat issue would be simply not auto-upgrading be? Yes all websockets servers would need to add a line to their code somewhere, but once they added that one line things would work. I don't want to have to opt-in to the preferred behavior for all future APIs just because it was inconvenient once to add a line when making a major version upgrade once. |
I am against breaking upgrades. The upgrade path should let people use the
|
I didn't mean to hijack the discussion like this, perhaps this question should have been posted on the mailing list instead. I still don't understand what "auto-upgrading" means in this context - could someone please explain it? |
On 20 March 2013 18:14, Adam Malcontenti-Wilson
At present, in response to a "HTTP 101 Switching Protocols", the HTTP This behaviour assumes that you wish to Upgrade, which is clearly not |
@isaacs - Is this still on the roadmap for v0.12? do you have a sample implementation of your proposal? |
@tjfontaine, @isaacs ping? Would you please mind sharing your final decisions on the API here? |
Is this still on the schedule for 0.12? |
For what's it worth. I have pure-js HTTP and Websocket codecs that perform pretty well. With these you can define whatever API you want and not be dependent on what node core provides. You only need TCP or TLS streams to transform. The down side is you lose compat with existing code that depends on the node APIs. I'm currently using my codec to spawn HTTP servers in chrome apps and create smart-protocol node servers that serve HTTP and two other custom protocols on the same port by guessing the protocol via the first byte. The HTTP codec is open at https://github.com/creationix/http-codec. |
Unfortunately this is not on target for 0.12, I have moved it to 0.13 for now. |
Any plans for this to be integrated into node (io.js) next.0? I'm not seeing any pingbacks to this issue apart from nodejs/node#550 which seems to addressing a completely different feature. |
Also, to make sure I'm understanding this correctly, this would make it possible to mount WebSocket-based libraries like ws and engine.io as middleware (rather than having to pass them the http server)? |
From the brief amount I've read on this, it sounds good… |
It's inconvenient right now that we automatically upgrade all upgradeable requests and do
server.emit('upgrade', req, req.socket, bodyHead);
res.statusCode = 403; res.end('Sorry, no websockets for you!')
, since a response object is never created.server.emit('request')
orrequest.emit('response')
.And basically, this is all just to prevent the overhead of creating a response object, which is cheap anyway.
However, changing this interface too dramatically is going to break some stuff pretty badly.
Idea:
'request'
event signature to add the socket as well:server.emit('request', req, res, socket)
request.upgradeable = true
flag.req.upgrade()
then unhook all the HTTP machinery, and now it's an upgraded connection.req.upgrade()
, emit the'upgrade'
event on the server with the req, socket, and the bit of the body that has already been read in.'response'
event signature to add the socket as well:request.emit('response', response, socket)
response.upgradeable = true
flag.response.upgrade()
, then unhook all the HTTP machinery, and now it's an upgraded connection.response.upgrade()
, emit the'upgrade'
event on the request with the response, socket, and the bit of the body that has already been read in.This should be changed in v0.12. The
bodyHead
change might not be an issue, if we refactor the parser/etc to take advantage of streams2 more effectively, so that we don't automatically pull everything out of the socket readable stream object./cc @substack @jclulow
The text was updated successfully, but these errors were encountered: