You mention some valid points, which I believe I tackled in the branch, at the
cost of breaking API. I'm just going to explain how that works, but I currently
have no way of doing this _without_ breaking API.
On Friday 13 April 2012 at 00:38, Brian wrote:
> That's probably still not enough, because you need to be able to push data
> out the socket immediately after the actual HTTP response, ideally even in
> the same TCP frame if possible. So calling response.end() doesn't make sense,
> and letting that stuff be transparently queued doesn't make sense, because
> you still have immediate access to the raw socket and could inject websocket
> data before the HTTP 101 response has been written.
The idea is to not get access to the socket until after the response finishes
writing. On the branch, I solved this by having an alternate `end` taking a
callback that fired roughly after the ServerResponse `finish` event and
`res.detachSocket()`:
server.on('upgrade', function(req, res) {
res.writeHead(101, { 'Connection': 'Upgrade', 'Upgrade': 'Foobar' });
res.switchProtocols(function(socket) {
// write data
});
});
I should've verified this with a network capture, but I believe any data
immediately sent from that inner callback is also tacked onto the same TCP
frame. I assumed this because some of Node's tests on this actually test frame
by frame, and they're not failing. Writes from ServerResponse and the user
callback to `switchProtocols` happen within the same tick, I believe.
> The main thing you need is to wait until all previous pending requests have
> been fulfilled before emitting the 'upgrade' event.
This can be done, and can be done right now, if we really want to prevent
people from talking on the socket before other responses are written.
Though, there's not a client out there that actually pipelines requests; the
test case I made for this was purely theoretical. Koichi's idea on this was to
simply close the socket if someone tried. But in the end, none of it went
through because there's no real world issue.
> The only major remaining issue is that node assumes that the 'upgrade'
> handler is going to take over the connection and there's no way for that
> handler to respond with, say, HTTP 404 and continue using the HTTP/1.1
> keep-alive connection for further HTTP requests.
Definitely, but you don't want to continue using the connection, because it is
no longer a keep-alive connection after the upgrade request. For the same
reason we send inner-protocol stuff in a single frame, the client may have done
so as well.
>
> Maybe just provide the ServerResponse object to to the 'upgrade' event
> handler the upgrade event handler would ONLY use it if it decided NOT to
> follow through with the HTTP Upgrade request? i.e. res.writeHead(101) would
> be illegal. But your race condition is solved by waiting until all prior http
> requests have been responded to before emitting the 'upgrade' event.
Then, where is the 101 response written? And how does that code decide if the
callback is going to cancel, before the callback is called? (Once it _is_
called, your socket is gone.)
The race condition is theoretical, and doesn't appear anywhere in practice.
Perhaps I emphasized it too much. I'm in it for the response object. :)
--
Stéphan Kochen
Two Screen, Angry Bytes
