This repository has been archived by the owner. It is now read-only.

Response object for HTTP Upgrade/CONNECT #3036

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
9 participants

I started a thread on nodejs-dev about this, but it looks like my posts are still in the moderator queue. Will link it here once it gets through.

Briefly:

  • The first commit is a test showing that a pipelined Upgrade request to a Node server can cause an out-of-order response. This is because the upgrade handler gets immediate access to the socket, while other requests may still be processing async.
  • The second commit fixes this by providing a response object even for Upgrade, that is also queued properly along with other responses.
  • In addition to fixing the test, I find it makes sense to have a response here. Per the spec, CONNECT and Upgrade must be followed by a proper response, before handover.

Regarding make test, I have one failing test both before and after this patch set, namely simple/test-http-full-response. I consider this a platform irk. make test-debug plain crashes the test suite on simple/test-repl (also before and after), but a fair amount of HTTP tests do pass.

stephank commented Apr 3, 2012

I have a branch of einaros/ws on top of this, with working tests and examples:

https://github.com/stephank/ws/tree/upgrade-response

stephank commented Apr 4, 2012

More branches of websocket projects:

These are all examples of projects reimplementing basic ServerResponse functionality.


I noticed a recurring pattern, along the lines of:

socket.on('data', dataListener);
dataListener(upgradeHead);

Perhaps we can simply emit upgradeHead as the first data event on the socket, after the switchProtocols callback finishes? (Or do we have to do this in a nextTick? I'm not sure if that guarantees ordering of events when new data arrives quickly.)

stephank commented Apr 4, 2012

I went ahead and rebased on current master, and implemented the earlier thought of simply emitting upgradeHead as a data event.

The branches of ws and faye-websocket have also been updated. (Sockjs didn't need any further updates.)

Member

piscisaureus commented Apr 4, 2012

@isaacs Could you flush the moderation queue for nodejs-dev? (or give me / @bnoordhuis admin rights)

koichik commented Apr 8, 2012

I think that @stephank's the new API is really nice. However, it breaks backward-compatibility. Because 'upgrade' event is widely used (by Socket.IO, especially), I do not want to change it.

Instead, I think that Node should close the connection when pipelined Upgrade/CONNECT request was detected. Because:

  • If Upgrade/CONNECT request is idempotent, client can resend the request.
  • If Upgrade/CONNECT request is not idempotent, client should not pipeline the request.

stephank commented Apr 8, 2012

Thanks for pitching in! That will, of course, solve the problem as well.

You're absolutely right when it comes to the API. I was still hoping we could move mountains by showing the API can be better, and demonstrating that I was willing to put work in projects currently using upgrade. The only reason I don't have a branch of Socket.IO is because I couldn't get its test suite running on Node 0.7.7.

Is there a minimum set of projects using upgrade that we want to ensure compatibility with? Would it help if we got their maintainers on board with this API change?

koichik commented Apr 8, 2012

@stephank - What WebSocket's client does pipeline the Upgrade request?

stephank commented Apr 8, 2012

There isn't any I'm aware of, that test is theoretical. Though it does actually work and triggers the bad HTTP server behavior on 0.6.14 and 0.7.7.

I'm in this for the response object, which can be very useful in combination with middleware. What led me to doing this was trying to add upgrade support to Connect middleware. See the related pull request for Connect, which implements it using the current Node API.

Briefly, that pull works, but a lot of middleware really wants to manipulate the response, e.g. add cookies, other headers, sometimes even abort the request (ie. basicAuth wants to reply with 401).

koichik commented Apr 10, 2012

@stephank - Sorry, I'm -1 to the PR. Because there is no problem in the real world, and the API is marked Stable (i.e. Backwards-compatibility is guaranteed.).
Closing.

@koichik koichik closed this Apr 10, 2012

koichik commented May 2, 2012

By the discussion in the ML, we will revisit in v0.9. Reopening.

@koichik koichik reopened this May 2, 2012

I just found this issue. I've been wanting this feature for a long time! What can I do to help? Basically my goal is to be able to handle websockets 100% from a handler function without needing access to the server object. This makes composing layers (like connect middlewares) possible when using websockets.

stephank commented Aug 8, 2012

  • This needs to be rebased on master. Renewed interest helps; I'll see about doing that this week. :)
  • On the mailing list, @theturtle32 raised the point of sending both the HTTP response and WebSocket handshake in a single frame. This pull request basically splits the write()s, which could be a problem. It looks like (in the tests) a single frame is sent because Nagle's is enabled by default, but it's likely that users will want to disable Nagle's as soon as they get the socket. We need figure out if this is a problem and, if so, how to fix it.
  • Other than that, I'm just waiting for feedback. :)

stephank commented Aug 9, 2012

Rebased. Test results still good.

I tried to create a test case for the issue with Nagle's. I basically disable Nagle's directly after switchProtocols, then write the response body to the HTTP Upgrade. A plain TCP client then counts data events to check for segmentation.

https://gist.github.com/3426025

The test currently passes. (ie. I only get one data event.)

Can someone with a bit more knowledge of net / libuv internals verify if this is a proper test case? I'm not sure if simply counting data events is enough verification.

adammw commented Nov 27, 2012

Any progress on this proposal getting into one of 0.9.x unstable releases?

The lack of a response object is making it hard for frameworks to implement upgrade request routing easily, meaning that upgrades are typically handed manually with a lot more work required.

As to the issue of backwards-compatibility that everyone seems to be hanging on, even with this proposed API break, wouldn't the only change required be something like this or am I missing something?

server.on('upgrade', function(req, socket, head) {
  if(!(socket instanceof net.Socket))
    socket = req.connection;

  //... legacy code that uses socket directly ...
})
Member

bnoordhuis commented Dec 28, 2012

PR no longer applies. Is this still an issue with master and if so, why?

@adammw: Sure, but API compatibility means things have to stay working without code changes to the libraries/applications using the API.

@bnoordhuis: It is still an issue. This is a proposal to change the upgrade/connect event API to no longer receive plain sockets, but a Response object. The PR is an implementation.

I'll take a look at another rebase on current master.

Here we go, rebased against c6e958d. Some things don't sit quite right with me, though:

  • Both master and this branch pass tests on a release build on my Mac. Tests on a debug build fail, however, even on plain master, so I have no comparison there. (test-debug-brk-file hangs, tried running some others manually and got assertion failures.)
  • http.js still uses old-style streams and pretty much forces this on servers using upgrade requests. (I'm guessing this is well known.) Right now, I'm still pausing/resuming the socket and injecting the head buffer back in using socket._readableState.onread() after the switch callback had its chance to install handlers. I think eventually, the onread() should be done before the switch callback, so that the callback gets an immediately readable stream?

Rebased on current master (8e311d2). Now using stream.push(). I succeeded in running make test-debug on my Mac this time, with the exact same results on master and this branch. (Several seemingly unrelated failures.)

Remaining points:

  • Waiting on new-style streams in http.js. (issue #4488)
  • Would still appreciate comments on separate write()s, as per this earlier comment.

Right, with #4488 closed, I rebased on master (926c90b) again. The socket.pause() and socket.resume() calls are gone. I now also socket.push() the upgradeHead before the switchProtocols() callback. Test results are good.

Remaining:

Should @stephank keep working on this? I'd hate to see him do all this work if there is no intention to accept it. This is a feature I would really like, but it is a change to the HTTP API which is stable now.

@stephank, is your change backwards incompatible with any existing node code? And if so, how significant is the change?

My approach of late has been to replace the http module entirely in userspace. (see web in npm).

@creationix Yes, it's an API change to the upgrade and connect event callbacks.

It's the difference between this server we have right now:

server.on('upgrade', function(req, sock, head) {
    sock.write('HTTP/1.1 101 Switching Protocol\r\n' +
               'Connection: Upgrade\r\n' +
               'Upgrade: Echo\r\n\r\n');

    sock.push(head);
    sock.on('readable', function() {
        var chunk = sock.read();
        sock.write(chunk);
    });
});

And this server on my branch:

server.on('upgrade', function(req, res) {
    res.writeHead(101, {
        'Connection': 'Upgrade',
        'Upgrade': 'Echo'
    });

    res.switchProtocols(function(sock) {
        sock.on('readable', function() {
            var chunk = sock.read();
            sock.write(chunk);
        });
    });
});

A server that handles both is slightly finicky, though:

server.on('upgrade', function(req, resOrSock, head) {
    var headers = {
        'Connection': 'Upgrade',
        'Upgrade': 'Echo'
    };

    if (resOrSock.switchProtocols) {
        resOrSock.writeHead(101, headers);
        resOrSock.switchProtocols(setupSock);
    }
    else {
        resOrSock.write('HTTP/1.1 101 Switching Protocol\r\n' +
                        Object.keys(headers).map(function(name) {
                            return name + ': ' + headers[name] + '\r\n';
                        }).join('') +
                        '\r\n');
        resOrSock.push(head);
        setupSock(resOrSock);
    }

    function setupSock(sock) {
        sock.on('readable', function() {
            var chunk = sock.read();
            sock.write(chunk);
        });
    }
});

There's now a package upgrade-ex in NPM that implements this stand-alone on top of Node.js 0.9.6.

@adammw suggested I look into backwards compatibility, but this is kind of the reverse. I can transform an 'old style' upgrade into a 'new-style' this way, but I've not yet figured out how to make 'old style' work on top of 'new style'.

isaacs commented Jan 18, 2013

@stephank So, it seems like the main blocker is that now you do on('upgrade', function(req, socket, head) {}) and with the patch, you'd do on('upgrade', function(req, res) {}). Why not making the new signature on('upgrade', function(req, socket, head, res) {})? It's a bit ugly, but would keep backwards compatibility. Changing event signatures is very destabilizing.

adammw commented Feb 17, 2013

Changing event signatures is very destabilizing.

That's worrying in of itself.

on('upgrade', function(req, socket, head, res) {}) is too ugly for my liking.

My suggestion is using different event name is used in place of upgrade (upgradeEx, upgrade-req, upgrade2, etc.) like is done in the update-ex npm package. Depending on which events have listeners depends on which behaviour is taken within the http module. And since they have different names, this also has the plus of being able to be detected which is being listened to, and allowing different signatures.
In the meantime, the upgrade event can still work as before but with a warning when a listener is attached telling the user/maintainer to upgrade their package to use the new event and event signature.

I really like this proposal, and think it works a lot better than the existing upgrade event behaviour, although I worry that now it is a npm module its inclusion into the nodejs core may be questioned, even though it is an improvement/replacement for existing functionality.

And considering the breakage and updates that will be needed from modifications of streams in 0.9/0.10, I think considering this sooner would be better than later.

Can one of the admins verify this patch?

adammw commented Mar 19, 2013

Seems like there may now be some motivation to make some changes to upgrade, but in a rather different way at #4813.

The last push contains a rebase on master (46da8c2), and reworded commit messages.

I had to change test-http-upgrade-client2.js, because it was doing really strange things that I'm not sure are valid on current node versions any way.

@adammw adammw referenced this pull request in senchalabs/connect Jun 25, 2013

Closed

A proposal for handling upgrade requests. #506

Rebase against current master (b255f4c).

I used to have a commit that removed upgradeHead on both the HTTP server and client, but master has since deprecated it and replaced it with an empty buffer for compatibility.

So the commit was no longer necessary, and I'm no longer touching the HTTP client code. I do, however, remove the empty buffer on the HTTP server, considering this is an API change any way.

I also removed the commit that fixed test-http-upgrade-client2.js, which also seems fixed in master already.

Member

bnoordhuis commented Jun 25, 2013

@stephank What benefit does this change have over the current approach?

Can't see why jenkins is failing, but I believe these are not failures related to the pull.

@bnoordhuis In short: middleware. See also #4813.

Member

bnoordhuis commented Jun 25, 2013

Right. @isaacs, this one's for you.

stephank added some commits Mar 29, 2012

http: failing test for pipelined upgrade
There are no known real world clients that actually send an upgrade
request pipelined after a regular request. Nonetheless, this tests
conformance to the standard.
http: upgrade/connect with response object
Introduces `switchProtocols()` on the response, which is an alternate
`end()` for responses that hijack the socket.

The common part for building a ServerResponse, shared with regular
requests, is extracted to a `createResponse()` function.

stephank commented Aug 6, 2013

Rebase against current master (b26d346). You guys are really good at breaking my stuff for years on end. :(

upgradeHead removal was apparently reverted again on master. I didn't add it back to the client.

@ghost ghost assigned isaacs Aug 9, 2013

isaacs commented Aug 9, 2013

@stephank Sorry for the absurd delay on this. I will review this in more detail very soon (today or Monday).

The main blocker that I can see right off the bat is that we probably can't get away with changing the server.on('upgrade') signature in a backwards-incompatible way (hence the ugly compromise in #4813). That being said, the core pipelining-related bug here definitely needs to be fixed, and #4813 is important, so I think at least most of the work you've done will be relevant and useful (even just the test is really great).

Thanks again, and my apologies for letting it linger so long. This is next in my queue after the OutgoingMessage refactoring.

@isaacs Thanks for taking a look.

Note though, the test is more or less theoretical. It definitely triggers bad behaviour, but I'm not aware of this ever happening in practice, with real clients.

I'm curious how you'll end up tackling #4813. Middleware is the important issue for me here.

stephank commented Sep 4, 2013

If my understanding is correct, there is no intention of merging this, and I have no intention of updating it any further.

Unless you guys want to keep this issue around for a hypothetical 2.0, you may as well close it.

Path ahead seems to be a separate module and getting people to use that. I might at some point in the future revive upgradeEx, but I'm currently otherwise occupied.

Owner

jasnell commented May 19, 2015

Keeping this around and open for 2+ years after the original poster gave up on updating it isn't helpful. Looks like there's been recent albeit not particularly active discussion on it on the io.js side. Given that it's unlike this specific PR will ever land in v0.x, closing. Can reopen if necessary.

@jasnell jasnell closed this May 19, 2015

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Oct 6, 2015

deps: backport 1ee712a from V8 upstream
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{#31111}

PR: #3036
PR-URL: nodejs/node#3036
Reviewed-By: Ben Noordhuis <ben@strongloop.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Oct 6, 2015

src: fix --abort-on-uncaught-exception
Revert 0af4c9e, parts of
921f2de and port
nodejs#25835 from v0.12 to
master so that node aborts at the right time when an error is thrown
and --abort-on-uncaught-exception is used.

Fixes #3035.

PR: #3036
PR-URL: nodejs/node#3036
Reviewed-By: Ben Noordhuis <ben@strongloop.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Oct 8, 2015

deps: backport 1ee712a from V8 upstream
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{#31111}

PR: #3036
PR-URL: nodejs/node#3036
Reviewed-By: Ben Noordhuis <ben@strongloop.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Oct 8, 2015

src: fix --abort-on-uncaught-exception
Revert 0af4c9e, parts of
921f2de and port
nodejs#25835 from v0.12 to
master so that node aborts at the right time when an error is thrown
and --abort-on-uncaught-exception is used.

Fixes #3035.

PR: #3036
PR-URL: nodejs/node#3036
Reviewed-By: Ben Noordhuis <ben@strongloop.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Nov 5, 2015

deps: backport 1ee712a from V8 upstream
Backport 1ee712ab8687e5f4dec93d45da068d37d28feb8b from V8 upstream.

Original commit message:

  Add SetAbortOnUncaughtExceptionCallback API

  The --abort-on-uncaught-exception command line switch makes
  Isolate::Throw abort if the error being thrown cannot be caught by a
  try/catch block.

  Embedders may want to use other mechanisms than try/catch blocks to
  handle uncaught exceptions. For instance, Node.js has "domain" objects
  that have error handlers that can handle uncaught exception like
  following:

  var d = domain.create();

  d.on('error', function onError(err) {
    console.log('Handling error');
  });

  d.run(function() {
    throw new Error("boom");
  });

  These error handlers are called by isolates' message listeners.

  If --abort-on-uncaught-exception is *not* used, the isolate's
  message listener will be called, which will in turn call the domain's
  error handler. The process will output 'Handling error' and will exit
  successfully (not due to an uncaught exception). This is the behavior
  that Node.js users expect.

  However, if --abort-on-uncaught-exception is used and when throwing an
  error within a domain that has an error handler, the process will abort
  and the domain's error handler will not be called. This is not the
  behavior that Node.js users expect.

  Having a SetAbortOnUncaughtExceptionCallback API allows embedders to
  determine when it's not appropriate to abort and instead handle the
  exception via the isolate's message listener.

  In the example above, Node.js would set a custom callback with
  SetAbortOnUncaughtExceptionCallback that would be implemented as
  following (the sample code has been simplified to remove what's not
  relevant to this change):

  bool ShouldAbortOnUncaughtException(Isolate* isolate) {
    return !IsDomainActive();
  }

  Now when --abort-on-uncaught-exception is used, Isolate::Throw would
  call that callback and determine that it should not abort if a domain
  with an error handler is active. Instead, the isolate's message listener
  would be called and the error would be handled by the domain's error
  handler.

  I believe this can also be useful for other embedders.

  BUG=

  R=bmeurer@chromium.org

  Review URL: https://codereview.chromium.org/1375933003

  Cr-Commit-Position: refs/heads/master@{#31111}

Ref: #3036
PR-URL: nodejs/node#3481
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.