Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promises on prerequisites makes handler to swallow exceptions #2771

Closed
nakardo opened this issue Sep 17, 2015 · 14 comments
Closed

Promises on prerequisites makes handler to swallow exceptions #2771

nakardo opened this issue Sep 17, 2015 · 14 comments
Assignees
Labels
bug Bug or defect

Comments

@nakardo
Copy link

nakardo commented Sep 17, 2015

I've this issue when using Promises on a prerequisite handler and throwing an error from a handler.

var Promise = require('promise');


exports.register = function (server, options, next) {

    var pre = function (request, reply) {

        return reply(Promise.resolve('value'));
    };

    server.route({
        method: 'GET',
        path: '/bar',
        config: {
            pre: [
                { method: pre, assign: 'pre' }
            ],
            handler: function (request, reply) {

                // throws here will get swallowed

                throw new Error('will timeout');
                return reply();
            }
        }
    });

    return next();
};

exports.register.attributes = {
    pkg: require('./package.json')
};

Replying with a literal value on the pre() does it make it work. Using Promises on the handler will too, but it sounds more like a workaround to me.

Here's the dependencies I'm using:

[email protected] node_modules/code
└── [email protected]

[email protected] node_modules/promise
└── [email protected]

[email protected] node_modules/hapi
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] ([email protected])
├── [email protected] ([email protected], [email protected], [email protected])
├── [email protected] ([email protected], [email protected])
└── [email protected] ([email protected])

[email protected] node_modules/lab
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected]
├── [email protected] ([email protected])
├── [email protected] ([email protected])
├── [email protected] ([email protected], [email protected], [email protected], [email protected])
├── [email protected] ([email protected], [email protected], [email protected], [email protected])
├── [email protected] ([email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected])
└── [email protected] ([email protected], [email protected], [email protected])

I can upload an small repo with a few tests if it helps.

@devinivy
Copy link
Member

I've experienced something like this too while playing around. Possibly related to outmoded/discuss#155.

@hueniverse
Copy link
Contributor

Since I don't use promises I can't really help you. Maybe someone who knows promises well can look into it. I'll leave this open for a bit.

@devinivy
Copy link
Member

@dmacosta if you put a repo up with some tests, I'll definitely take a peek.

@nlf
Copy link
Member

nlf commented Sep 17, 2015

my gut says this is because of domains. likely something is causing the request handler's domain to get lost.

@nlf
Copy link
Member

nlf commented Sep 17, 2015

@devinivy i'm online in gitter if you want a second pair of eyes on this when you get around to checking it out

@nakardo
Copy link
Author

nakardo commented Sep 17, 2015

@nlf
Copy link
Member

nlf commented Sep 17, 2015

confirmed this is a domains issue, i think i have a fix

@nlf
Copy link
Member

nlf commented Sep 17, 2015

ok, i don't have a fix, but i can definitely confirm it's an issue with domains..

adding logging to the error handler in lib/protect.js reveals the error isn't caught at all. logging process.domain within the handler that is run after a normal prerequisite gives a domain object, however in a handler that is run after a prerequisite that returns a promise, it's undefined.

that's as far as i got. domains are hard.

@devinivy
Copy link
Member

Yeah, I spent about 15mins with this and didn't get far at all. The promise library used (https://github.com/then/promise) does have a note about domains in the README worth looking at.

@hueniverse hueniverse added the bug Bug or defect label Sep 25, 2015
@hueniverse hueniverse self-assigned this Sep 25, 2015
@hueniverse
Copy link
Contributor

I'm going to label this as a bug but close it because we can't do anything about these issues. I am going to add a domains-free option soon.

@devinivy
Copy link
Member

Worth adding that I revisited this. The "done" handler that hapi calls for the promise escapes the domain (i.e., onDone in promiseFromPre.then(onDone, onDone)), but there was no way I could fix it. Different promise libraries behaved slightly differently, too. The promise library used in this particular example looks like it executes the callbacks outside a domain intentionally via the asap module.

@hueniverse
Copy link
Contributor

You can now try turning off domains if you want.

@rubennorte
Copy link
Contributor

I ended up in this issue after loosing the active domain in my handlers. After some investigation I tested replacing native promises with Bluebird and it works fine.

I created a gist with examples of this behaviour in case it helps someone: https://gist.github.com/rubennorte/f10d2f6ec376b28589a98bb17ef6ae85

@kanongil
Copy link
Contributor

Node will support domain bindings for promises, which has already been implemented in the master branch, see nodejs/node#12489.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug or defect
Projects
None yet
Development

No branches or pull requests

6 participants