Support Upgrade requests

[defunctzombie]

Using on-finished on a request that is then upgraded causes it to erroneously detect that the request is finished.

Below is a minimal example which shows this failure:

var bouncy = require('bouncy');
var on_finished = require('on-finished');

// receiving server that we will proxy to
var engine = require('engine.io');
var server = engine.listen(8080);
server.on('connection', function(socket){
    socket.send('utf 8 string');
});

// proxy server
var server = bouncy(function (req, res, bounce) {
    on_finished(req, function() {
        console.log('finished');
    });

    bounce(8080);
});
server.listen(8000, function() {
    var socket = require('engine.io-client')('ws://localhost:8000', {
        transports: ['websocket']
    });
    socket.on('open', function(){
        console.log('open');
    });
    socket.on('message', function(data) {
        console.log(data);
    });
    socket.on('error', function(err) {
        console.log('error', err);
    });
});

npm install bouncy engine.io engine.io-client

Bouncy is used here since it handles both regular requests and upgrade requests. And engine.io has been hard pinned to use on the websocket transport (same as using raw websockets basically).

[defunctzombie]

Notice that the "finished" string is printed before "open" and the received message which should not be the case if working as expected.

[dougwilson]

Just curious: what is the behavior for res?

[defunctzombie]

I added a handler for res. Both print finished before any other output.

[dougwilson]

K. I wonder if isFinished is a Boolean or undefined at the moment on_finished is called.

[dougwilson]

Ok, so far what I see is that when on_finished is called, req.complete === true. Still investigating.

[dougwilson]

As far as I can tell so far, this is how Node.js works; it considers the HTTP request/response to be done before the socket is handed over to continue with WebSockets. This is probably technically true, because "finished" in the Node.js sense is that of the actual HTTP message on the socket, and the HTTP message completely finishes once the WebSockets handshake occurs (even though you can now continue to communicate over the raw TCP socket--which is no different than pipelined HTTP messages).

Basically this is what I'm seeing:

1. TCP socket open
2. Client sends HTTP request to upgrade to WebSocket
3. Server reads the HTTP request headers, and since there is no request body, the HTTP request is now finished.
4. Server makes the HTTP response to say that the channel is being upgraded to WebSockets
5. Once this response is sent, the HTTP response is finished.
6. Now the TCP socket is being used for WebSocket communication and no longer HTTP

Basically, I'm seeing this as the expected behavior, in so much as this library says when a HTTP request or HTTP response is finished, which both are complete before socket.on('open', listener) would fire, since both need to be finished before the TCP channel becomes an open WebSocket.

[defunctzombie]

Gotcha. Thanks for looking into it. Will consider it correct behavior in terms of http req/res cycles.

Would it be appropriate to add something to the readme about this?

[dougwilson]

Would it be appropriate to add something to the readme about this?

Absolutely. I'm still doing a bit more research on what Node.js is doing and stuff, but it sounds like this may turn out to be a documentation issue, does that sound right to you :)?

I also wonder if it would be worth adding something to the lib for WebSockets (currently you would have to socket.on('close', listener).on('error', listener) after the upgrade is established--and that's just guesses without looking too deep into it).

[defunctzombie]

Absolutely. I'm still doing a bit more research on what Node.js is doing and stuff, but it sounds like this may turn out to be a documentation issue, does that sound right to you :)?

Depends on the goals of the lib. If the goals are to handle the basic request/response cycle, then yea documentation is all that is needed.

If the goal is to support http requests in general, one could argue that once upgraded, the socket is not yet finished nor has it error'd. I am not even sure it is true to say the response has finished. Because of this "what do we do" uncertainty, I would lean more towards saying just document the behavior with upgrade requests and say the user is responsible for detection after upgrade.

The counter argument to the above is that everyone will likely have to write the same socket close handler code if they want to support websockets so why not just do it in the lib that handles a general request/response lifecycle.

I personally am ok with documentation since it is really just a matter of knowing not to over-use the lib.

[dougwilson]

(as a note to myself, this is the interesting part of core: https://github.com/joyent/node/blob/v0.10.26/lib/http.js#L1970-L1991)

[dougwilson]

I agree with all your points, which is also why I'm in agreement with what the solution is to be here :) From looking around in the core, it doesn't so far like it's going to be easy (assuming it's possible) to get the after HTTP-lifetime cycle included in the "finished" on this module.

As far as the way Node.js core is organized, I would say that the current limitation is that this module only supports req and res objects that originate from the request event on HTTP servers (your example was passing in a req object that came from the upgrade event). I'm not saying that we can't for sure, but I was just trying to narrow down what exactly we were currently working with in the module and why this case ended up acting differently.

[dougwilson]

I went ahead and updated the topic for this issue now that I understand what is going on with the report :)

[dougwilson]

It seems we should be able to detect this based on msg.upgrade === true and then defer to the state of the socket, rather than the state of the HTTP message.

[dougwilson]

Currently the biggest debate in my head right now is this, if you can weigh in on your opinion:

1. Should "finished" for this module be "HTTP message finished" or "logical thing finished"?
2. Should we support both or only one (and if both, which should be the default?)?

[defunctzombie]

I would say that it should be: "logical" and "both default" iff the code changes do not add lots of additional state management and edge conditions.

But I am also strongly tempted to say: "http message finished" since folks using websockets or other longer lived connections will more likely be attaching themselves to the events on the socket. Also, keeps this module simpler and focused on the request/response lifecycle of 98% usecase for users of this module. Only happened to me because of my rather esoteric use of this module in localtunnel-server.

[dougwilson]

Yea. And I've been thinking about it more as well and I'm kinda leaning on the "local" one being the only--mainly because the Upgrade requests are already split apart from "normal" requests in the way of then being different events, and of course if you're going to use this module on something from the upgrade event, the only logical thing you would want know is when the whole thing was finished :)

I've been playing around with the code and it doesn't seem like the change is really big at all--just lots more tests in the test suite, mainly :)

[dougwilson]

Darn these tests, lol.

[jonathanong]

what's the issue? does bouncy not differentiate between different types of requests like checkContinue and upgrade?

[dougwilson]

It's not an issue with bouncy or anything. This is a request to support Upgrade requests by not triggering the finished until the socket is closed, rather than when the HTTP message is finished (and in node.js, the Upgrade requests are never not finished).

[defunctzombie]

@jonathanong right, bouncy just handles them both in this single function. I could have made the example using the core http module upgrade event instead but wanted to show a case where a user might try to use this module but get confused without realizing websocket upgrade requests were not supported.

[dougwilson]

Yep. I'm just writing the tests in core, but I agree that when combined with bouncy is a great example of the behavior being odd. Typically, though, people are going to want to know when the WebSocket is finished for WebSocket requests; what this module does for them right now is useless anyway :)

[dougwilson]

Ok, sorry for the long delay on this issue. I have read up a lot about this to figure out what would make the most sense. In the end, accounting for the actual behavior of Upgrade (not specific to WebSockets, but in the context of HTTP itself), I have come to find that the current behavior is the most appropriate. Because of this, it seems the solution to this issue will be to provide more documentation to explain how this module interacts with the underlying Upgrade mechanism in HTTP/1.1 as well as the CONNECT method.

[dougwilson]

And for the record, I have basically been debating between keeping the existing behavior and documenting, or using this behavior:

• isFinished(req) will be true after the request's body (if there is one) has been read. This means that in the case of a WebSocket, isFinished(req) will be true for the duration of the socket. If this lasted beyond that point, then onFinished(req, cb) would not be useful in reading the actual request body.
• isFinished(res) will depend on if the connection was actually upgraded. If it was not upgraded, then it will act as usual, otherwise it will not be finished until the underlying socket is eventually closed.

[defunctzombie]

👍