Exception from a promise in Zippy view causes a hang

VERIFIED FIXED

Status

P2
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: kumar, Assigned: ozten)

Tracking

x86
Mac OS X
Points:
---

Details

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();
    });
};

Updated

5 years ago
Priority: -- → P4
I'm seeing this hang real requests on a dev server, not just tests now. Still can't figure out why.
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
Priority: P4 → P2
Summary: Exception from a promise in Zippy view causes tests to hang → Exception from a promise in Zippy view causes a hang

Updated

5 years ago
Assignee: nobody → ozten.bugs
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
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.

Updated

5 years ago
Version: 1.4 → 1.5
Is that bug still relevant now that we migrated to `when` promises?

Comment 7

5 years ago
I don't know, is it?
Flags: needinfo?(kumar.mcmillan)
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
Flags: needinfo?(kumar.mcmillan)

Comment 9

4 years ago
You reported it, any chance you could verify based on your last comment?
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(kumar.mcmillan)
Resolution: --- → FIXED
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)
Status: RESOLVED → VERIFIED
Flags: needinfo?(kumar.mcmillan)
You need to log in before you can comment on or make changes to this bug.