On Thu, Apr 12, 2012 at 11:41 PM, Stéphan Kochen <step...@angrybytes.com>wrote:
> 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.
As a counterpoint, my boss's boss at my previous job had enabled pipelining
in Firefox through about:config editor because he was trying to improve
performance, and we ended up with bug reports for our app because our J2EE
app didn't respond in order with HTTP/1.1 pipelining. We couldn't
reproduce the problems he was having on his machine no matter what he did.
It took us weeks to figure out that it was because he had pipelining
enabled in his browser.
> 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.)
>
WebSocket-Node writes the 101 response manually via socket.write().
https://github.com/Worlize/WebSocket-Node/blob/31c47d1622c6f626235eed3ed516c3d2afe1a91d/lib/WebSocketRequest.js#L223
I suppose that's not ideal from an API perspective. You could do
res.writeHead(101) immediately followed by socket.write(), but that
probably wouldn't end up in the same TCP frame if Nagle was disabled.
Brian
