938352 - Exception from a promise in Zippy view causes a hang

Status: VERIFIED FIXED

(Marketplace Graveyard :: Payments/Refunds, defect, P2)

Description

[Kumar McMillan [:kumar]]

An exception passed like next(err) from a promise causes tests to hang. I have no idea why! To reproduce:


View code:


var Q = require('q');

exports.get = function (req, res, next) {
  return Q.promise(function() {
      // stuff
    })
    .then(function() {
      throw new Error('not an HTTP error');
    })
    .fail(function(err) {
      // You'll see this log statement before the hang.
      console.log('got err', err);
      next(err);
    });
};


Test code:

exports.testSomething = function(t) {
  client.get().expect(200)
    .end(function(err, res) {
      t.ifError(err);
      t.done();
    });
};

Comment 1

[Kumar McMillan [:kumar]]

I'm seeing this hang real requests on a dev server, not just tests now. Still can't figure out why.

Comment 2

[Kumar McMillan [:kumar]]

Bumping priority because this happens on real requests, not just tests.

I dug into it once but got lost. Something in here seemed to be ignoring the error: https://github.com/mcavage/node-restify/blob/master/lib/server.js#L660

Comment 3

[Austin King [:ozten]]

I'm looking into this.

Thoughts off the top of my head:
Promises can be a good choice were having a simple, chainable API is the highest priority. The down sides are complexity and a large increase in the size of implementation code.

Comment 4

[Austin King [:ozten]]

I want to dig deeper, but here is what I've got so far...

In case of an exception at line 53 of payments.js
(https://github.com/mozilla/zippy/blob/master/lib/payments.js#L53)

The res object is never written too, nor is send or end called... so restify hasn't called our callback and it hangs.

This can be fixed by adding this as the first line of the fail block:

    res.send(301);

HOWEVER: there is still some voodoo here. send(301) works, but send(500) has restify log it's own errors about undefined headers. res.send(301) isn't really the right thing to do, so I want to dig deeper into why send(301) works but other ways of finishing the HTTP response don't.

Comment 5

[Kumar McMillan [:kumar]]

We should be able to throw an error anywhere in our app and have that return a 500 to the user. As I understand it, restify claims to support this behavior.

Comment 6

[David Larlet [:davidbgk]]

Is that bug still relevant now that we migrated to `when` promises?

Comment 7

[Andy McKay]

I don't know, is it?

Comment 8

[Kumar McMillan [:kumar]]

I think it was a restify bug so moving to express might have fixed it. Someone can check by adding:

throw new Error('not an HTTP error');

to a then() like this: https://github.com/mozilla/zippy/blob/master/lib/products.js#L142

and running the test suite

Comment 9

[Andy McKay]

You reported it, any chance you could verify based on your last comment?

Comment 10

[Kumar McMillan [:kumar]]

w00t! I did not get a hang when doing comment #8 so I think it's fixed. Instead I got an error report which is exactly what I want to see:

Error: [Error: not an HTTP error]
Fatal error: Cannot read property 'body' of undefined
TypeError: Cannot read property 'body' of undefined
    at /Users/kumar/dev/zippy/test/suite/products.test.js:164:20
    at spreadArgs (/Users/kumar/dev/zippy/node_modules/super-request/node_modules/comb/lib/base/functions.js:26:17)
    at Object._onImmediate (/Users/kumar/dev/zippy/node_modules/super-request/node_modules/comb/lib/promise.js:100:25)
    at processImmediate [as _immediateCallback] (timers.js:336:15)