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

Confusing warning #871

Closed
oliversalzburg opened this Issue Nov 16, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@oliversalzburg
Contributor

oliversalzburg commented Nov 16, 2015

I just enabled warnings in one of our projects and I keep receiving one that is highly confusing.

Warning: a promise was created in a  handler but was not returned from it
    at Queue$enqueue [as enqueue] (...\core\src\job\queue.js:32:9)

When I jump to that location, I see this code:

Queue.prototype.enqueue = function Queue$enqueue( job ) {
    return new Promise( function resolver( resolve, reject ) {…} );
};

Line 32 is the one with the return statement. So, what is the warning trying to tell me? From what I can tell, the promise is returned.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 17, 2015

Owner

Do you have more lines in the stack trace? Just because enqueue returns the promise, doesn't mean its callers or callers' callers etc do.

For instance:

.then(function() {
     queue.enqueue(...);
});

Even though enqueue returned the promise, the then handler is not returning it.

Owner

petkaantonov commented Nov 17, 2015

Do you have more lines in the stack trace? Just because enqueue returns the promise, doesn't mean its callers or callers' callers etc do.

For instance:

.then(function() {
     queue.enqueue(...);
});

Even though enqueue returned the promise, the then handler is not returning it.

@oliversalzburg

This comment has been minimized.

Show comment
Hide comment
@oliversalzburg

oliversalzburg Nov 17, 2015

Contributor

Yeah, the trace is a lot longer actually.

Warning: a promise was created in a  handler but was not returned from it
    at Queue$enqueue [as enqueue] (…\core\src\job\queue.js:32:9)
    at requestHandler (…\core\src\modules\requestHandler.js:58:19)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at next (…\core\node_modules\express\lib\router\route.js:131:13)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:91:12)
    at next (…\core\node_modules\express\lib\router\route.js:131:13)
    at checkAuthenticationResult (…\core\src\authentication\authentication.js:173:11)
    at complete (…\core\node_modules\passport\lib\middleware\authenticate.js:243:13)
    at …\core\node_modules\passport\lib\middleware\authenticate.js:250:15
    at pass (…\core\node_modules\passport\lib\authenticator.js:421:14)
    at Authenticator.transformAuthInfo (…\core\node_modules\passport\lib\authenticator.js:443:5)
    at …\core\node_modules\passport\lib\middleware\authenticate.js:247:22
    at IncomingMessage.req.login.req.logIn (…\core\node_modules\passport\lib\http\request.js:61:13)
    at Strategy.module.exports.strategy.success (…\core\node_modules\passport\lib\middleware\authenticate.js:228:13)
    at verified (…\core\node_modules\passport-http-bearer\lib\strategy.js:126:10)
    at checkResult (…\core\src\authentication\authentication.js:120:13)
From previous event:
    at Strategy.processBearerStrategy [as _verify] (…\core\src\authentication\authentication.js:107:6)
    at Strategy.authenticate (…\core\node_modules\passport-http-bearer\lib\strategy.js:132:10)
    at attempt (…\core\node_modules\passport\lib\middleware\authenticate.js:341:16)
    at authenticate (…\core\node_modules\passport\lib\middleware\authenticate.js:342:7)
    at enforceBearerToken (…\core\src\authentication\authentication.js:162:6)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at next (…\core\node_modules\express\lib\router\route.js:131:13)
    at Route.dispatch (…\core\node_modules\express\lib\router\route.js:112:3)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at …\core\node_modules\express\lib\router\index.js:277:22
    at Function.process_params (…\core\node_modules\express\lib\router\index.js:330:12)
    at next (…\core\node_modules\express\lib\router\index.js:271:10)
    at Function.handle (…\core\node_modules\express\lib\router\index.js:176:3)
    at router (…\core\node_modules\express\lib\router\index.js:46:12)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at trim_prefix (…\core\node_modules\express\lib\router\index.js:312:13)
    at …\core\node_modules\express\lib\router\index.js:280:7
    at Function.process_params (…\core\node_modules\express\lib\router\index.js:330:12)
    at next (…\core\node_modules\express\lib\router\index.js:271:10)

The line in requestHandler also returns: return jobQueue.enqueue( functionRef ). Even though it doesn't really make sense, as it's an express middleware, which doesn't care about promises.

However, the enqueue call has a catch handler that already captures any possible failures. So I don't understand what the mistake here is or how I should change our code to avoid it.

Contributor

oliversalzburg commented Nov 17, 2015

Yeah, the trace is a lot longer actually.

Warning: a promise was created in a  handler but was not returned from it
    at Queue$enqueue [as enqueue] (…\core\src\job\queue.js:32:9)
    at requestHandler (…\core\src\modules\requestHandler.js:58:19)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at next (…\core\node_modules\express\lib\router\route.js:131:13)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:91:12)
    at next (…\core\node_modules\express\lib\router\route.js:131:13)
    at checkAuthenticationResult (…\core\src\authentication\authentication.js:173:11)
    at complete (…\core\node_modules\passport\lib\middleware\authenticate.js:243:13)
    at …\core\node_modules\passport\lib\middleware\authenticate.js:250:15
    at pass (…\core\node_modules\passport\lib\authenticator.js:421:14)
    at Authenticator.transformAuthInfo (…\core\node_modules\passport\lib\authenticator.js:443:5)
    at …\core\node_modules\passport\lib\middleware\authenticate.js:247:22
    at IncomingMessage.req.login.req.logIn (…\core\node_modules\passport\lib\http\request.js:61:13)
    at Strategy.module.exports.strategy.success (…\core\node_modules\passport\lib\middleware\authenticate.js:228:13)
    at verified (…\core\node_modules\passport-http-bearer\lib\strategy.js:126:10)
    at checkResult (…\core\src\authentication\authentication.js:120:13)
From previous event:
    at Strategy.processBearerStrategy [as _verify] (…\core\src\authentication\authentication.js:107:6)
    at Strategy.authenticate (…\core\node_modules\passport-http-bearer\lib\strategy.js:132:10)
    at attempt (…\core\node_modules\passport\lib\middleware\authenticate.js:341:16)
    at authenticate (…\core\node_modules\passport\lib\middleware\authenticate.js:342:7)
    at enforceBearerToken (…\core\src\authentication\authentication.js:162:6)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at next (…\core\node_modules\express\lib\router\route.js:131:13)
    at Route.dispatch (…\core\node_modules\express\lib\router\route.js:112:3)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at …\core\node_modules\express\lib\router\index.js:277:22
    at Function.process_params (…\core\node_modules\express\lib\router\index.js:330:12)
    at next (…\core\node_modules\express\lib\router\index.js:271:10)
    at Function.handle (…\core\node_modules\express\lib\router\index.js:176:3)
    at router (…\core\node_modules\express\lib\router\index.js:46:12)
    at Layer.handle [as handle_request] (…\core\node_modules\express\lib\router\layer.js:95:5)
    at trim_prefix (…\core\node_modules\express\lib\router\index.js:312:13)
    at …\core\node_modules\express\lib\router\index.js:280:7
    at Function.process_params (…\core\node_modules\express\lib\router\index.js:330:12)
    at next (…\core\node_modules\express\lib\router\index.js:271:10)

The line in requestHandler also returns: return jobQueue.enqueue( functionRef ). Even though it doesn't really make sense, as it's an express middleware, which doesn't care about promises.

However, the enqueue call has a catch handler that already captures any possible failures. So I don't understand what the mistake here is or how I should change our code to avoid it.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 17, 2015

Owner

What about at checkAuthenticationResult (…\core\src\authentication\authentication.js:173:11) or at checkResult (…\core\src\authentication\authentication.js:120:13) ? Is that inside a then handler and returning the promise created back at enqueue

Owner

petkaantonov commented Nov 17, 2015

What about at checkAuthenticationResult (…\core\src\authentication\authentication.js:173:11) or at checkResult (…\core\src\authentication\authentication.js:120:13) ? Is that inside a then handler and returning the promise created back at enqueue

@oliversalzburg

This comment has been minimized.

Show comment
Hide comment
@oliversalzburg

oliversalzburg Nov 17, 2015

Contributor

That line is not in a .then handler, no. It's the last middleware before our actual request handler. Line 173 is just return next();, next here ending up as calling requestHandler.

Contributor

oliversalzburg commented Nov 17, 2015

That line is not in a .then handler, no. It's the last middleware before our actual request handler. Line 173 is just return next();, next here ending up as calling requestHandler.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 17, 2015

Owner

What is at at Strategy.processBearerStrategy [as _verify] (…\core\src\authentication\authentication.js:107:6)

Owner

petkaantonov commented Nov 17, 2015

What is at at Strategy.processBearerStrategy [as _verify] (…\core\src\authentication\authentication.js:107:6)

@oliversalzburg

This comment has been minimized.

Show comment
Hide comment
@oliversalzburg

oliversalzburg Nov 18, 2015

Contributor

Here's the entire function with line numbers:

Contributor

oliversalzburg commented Nov 18, 2015

Here's the entire function with line numbers:

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 18, 2015

Owner

checkResult is insode a then handler and calls done which ends up creating a promise further at enqueue but done is not returning it.

You can avoid the callback plumbing and the warning by using .asCallback:

.then(...).asCallback(done)

It will worry about turning rejections into err first callback and fulfillments into result callbacks, you just need to return the object from the .then

Owner

petkaantonov commented Nov 18, 2015

checkResult is insode a then handler and calls done which ends up creating a promise further at enqueue but done is not returning it.

You can avoid the callback plumbing and the warning by using .asCallback:

.then(...).asCallback(done)

It will worry about turning rejections into err first callback and fulfillments into result callbacks, you just need to return the object from the .then

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 18, 2015

Owner

Actually there is a bug where asCallback emits the warming, ill fix that today

Owner

petkaantonov commented Nov 18, 2015

Actually there is a bug where asCallback emits the warming, ill fix that today

@oliversalzburg

This comment has been minimized.

Show comment
Hide comment
@oliversalzburg

oliversalzburg Nov 18, 2015

Contributor

Wow, thanks a lot for your extensive help on this one :)

Contributor

oliversalzburg commented Nov 18, 2015

Wow, thanks a lot for your extensive help on this one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment