A proposal for handling upgrade requests.

[stephank]

This is a working implementation, with tests, for handling upgrade requests with connect middleware.

The basic premise is to reuse the existing middleware stack, but have a separate handler for these requests. With this pull, things work as follows:

var blog = function(req, res, next) {
  next();
}
blog.upgrade = function(req, sock, head, next) {
  next();
}

var app = connect();
app.use('/blog', blog);

Middleware without an upgrade handler is skipped. If no upgrade handler is found for an upgrade request, the request socket is closed. (This follows http.Server behavior, without an upgrade event listener.)

Possible issues:

• Existing middleware that already has an upgrade attribute will do funky stuff when an upgrade request is sent there.
• Applications that used to handle upgrade events on the http.Server themselves will fail, because Connect is always the first listener, and will close the socket.

[stephank]

Any thoughts on this?

In the meantime, I have a branch of sockjs-node with some middleware sugar that upstream is willing to accept. There's an example of mounting SockJS as middleware. (Compare to the non-Connect example.)

Something like ws would be mountable with a dummy HTTP server, or using the noServer option. Here's the latter derived from their README example:

var connect = require('connect');
var WebSocketServer = require('ws').Server;

function websocketMiddleware(callback) {
    var wss = new WebSocketServer({ noServer: true });
    var middleware = function(req, res, next) { next(); };
    middleware.upgrade = function(req, sock, head, next) {
        wss.handleUpgrade(req, sock, head, callback);
    };
    return middleware;
}

var app = connect();
app.use(websocketMiddleware(function(ws) {
    ws.on('message', function(message) {
        console.log('received: %s', message);
    });
    ws.send('something');
}));
app.listen(8080);

Though hopefully, upstream ws will also accept some sugar for this (along the lines of websocketMiddleware up there), but I haven't talked to them yet. Such things are easily implemented without a Connect dependency.

[TooTallNate]

I don't really see the big difference between this and simply listening for the app.on('upgrade') event. When would I need multiple "upgrade" middleware to do things incrementally to the connection?

[stephank]

Some of the stock middleware in Connect is potentially useful, but would of course need to be adapted to handle upgrade requests. I'm thinking of: vhost, query, cookie, basicAuth, and logger.

For me, just the routing would already be very useful.

[stephank]

Reusing existing middleware is definitely something to investigate here.

I started out by modifying the logger middleware. It'll treat an upgrade request as if with the immediate option, and log Upgrade instead of a status code. Here's the result of an upgrade and a regular request to examples/logger.js:

127.0.0.1 - - [Wed, 28 Mar 2012 13:59:06 GMT] "GET /favicon.ico HTTP/1.1" Upgrade - "-" "curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5"
127.0.0.1 - - [Wed, 28 Mar 2012 13:59:12 GMT] "GET /favicon.ico HTTP/1.1" 200 - "-" "curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5"

The stuff doesn't have tests, but then logger doesn't have any tests at all.

What's interesting here is that a normal HTTP response will always follow (usually 101), per the HTTP spec. But we cannot catch and log it, because Node doesn't provide us with a response object. We can create one, but I'm not sure what it'll do to the socket we so badly want.

(Probably also the reason why Node's default behavior, without an upgrade event handler, is to just close the socket; it doesn't have a neat way of generating a 404.)

[stephank]

Here's an experiment in adding a response object to Upgrade requests, right in Node's http.js: nodejs/node-v0.x-archive#3036

This would also allow middleware to add headers (e.g. cookie), and more easily abort the request without having to write the HTTP response out manually (e.g. basicAuth).

FWIW, I only just noticed that Node also has a connect event similar to upgrade, though that's a small thing to add support for if we really want to.

[stephank]

It was worth a shot, I think, but it looks like API changes to Node's http.js are right out at this point, with good reason.

So, some options:

• Stick close to the way Node currently deals with upgrade, ie. (req, sock, head). This is was this pull currently implements. I find it has limited use in middleware, and will be mostly useful in routing.
• Create a ServerResponse ourselves. I'm still fond of the idea of having a proper response object, so I set out on another branch that does this in Connect, rather than Node: https://github.com/stephank/connect/tree/upgrade-response
• Anything else?

The upgrade-response branch works very similar to what I originally proposed as an API change in Node.js, except that there's no separate handler:

var middleware = function(req, res, next){
  if (req.upgrade) {
    res.writeHead(101, { 'Connection': 'Upgrade', 'Upgrade': 'Dummy' });
    res.switchProtocols(function(sock) {
      sock.on('data', function(data) { /* ... */ });
    });
  } else {
    next();
  }
}
middleware.supportsUpgrade = true;
app.use(middleware);

But the beauty is that for middleware that simply wants to amend (logger) or cancel (basicAuth) the request, nothing much changes but fn.supportsUpgrade = true. See, for example, the logger commit.

(It's worth mentioning that on this branch, logger is also able to properly log the HTTP response, ie. no 'immediate'.)

[stephank]

I went through the stock middleware, and most of them will plain work if given a ServerResponse object.

On branch upgrade-response, these all work with no functional changes:

basicAuth, cookieParser, cookieSession, csrf, errorHandler,
logger, methodOverride, query, responseTime, session

I also have a version of vhost that works with slight changes, but is backwards compatible. (I'm mostly wondering if we should next(utils.error(404)), rather than just next(), in some cases.)

Middleware that doesn't make sense:

bodyParser, compress, directory, favicon, json, limit, multipart, static, staticCache, urlencoded

edit: Middleware that uses the request body doesn't make sense either, of course. Reverted those changes.

[igorw]

What's the status on this? It would be great if websocket libraries could be integrated with connect.

[stephank]

The Node.js pull-request on this has been reopened, but the plan there is to reconsider the idea for Node.js 0.9.

Nothing much here on the Connect front, unfortunately.

[mansona]

@stephank when you say "nothing much here on the connect front" does that mean that there is nothing that you guys can do to get around this? Are you blocked by the update/pull request in Node before you can do anything?

I'm in need of this feature because of an issue that I have in socket.io socketio/socket.io#879 obviously all connected ;)

[stephank]

Simply meant to say that there's no update.

I have an API proposal that can be implemented either at the Node level, or the Connect level. Node will consider this for 0.9, I've yet to hear from any of the maintainers here.

But I think it's unlikely that anything will happen until Node makes a decision, considering the risk that doing anything now in Connect will possibly result in APIs between Node and Connect diverging, for example if Node decides on a slightly different API later on.

[adammw]

Ref: nodejs/node-v0.x-archive#3036 nodejs/node-v0.x-archive#4813

[jonathanong]

reopen when those node issues are closed