Sequelize::sync causes warning: "a promise was created in a handler but was not returned from it" #4883

Closed
raffishquartan opened this Issue Nov 19, 2015 · 29 comments

Comments

9 participants
@raffishquartan

Calling Sequelize::sync for a pg DB or a mysql DB in 3.14.0 causes a warning log message, apart from the different dialect files the stack trace is the same:

Warning: a promise was created in a  handler but was not returned from it
    at ConnectionManager.disconnect (/home/christo/repos/sequelize/playground/tests/10-sync-warning-repro/node_modules/sequelize/lib/dialects/postgres/connection-manager.js:162:10)
    at ConnectionManager.$disconnect (/home/christo/repos/sequelize/playground/tests/10-sync-warning-repro/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:257:41)
    at /home/christo/repos/sequelize/playground/tests/10-sync-warning-repro/node_modules/sequelize/lib/dialects/abstract/connection-manager.js:223:16
From previous event:
    ...

This warning occurs for me in 3.14 but not in 3.13 (but that might just be because bluebird was updated to ^3.0.5 from ^2.10.1 between those two versions) with both pg and mysql. I haven't tried any other DB's. It doesn't matter whether force: true is passed as an option to sync or not.

Looking at the code referred to by the call stack there isn't an obvious place where a Promise is created without being returned. Reading around about this problem though (link1, link2) it might be that the stack trace is misleading.

There is a repro here, (permanent link to this commit).

@mickhansen

This comment has been minimized.

Show comment
Hide comment
@mickhansen

mickhansen Nov 19, 2015

Contributor

@faceleg is working on cleaning that up here: #4862
It doesn't affect anything, simply a warning.

Contributor

mickhansen commented Nov 19, 2015

@faceleg is working on cleaning that up here: #4862
It doesn't affect anything, simply a warning.

@mickhansen mickhansen closed this Nov 19, 2015

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Nov 19, 2015

Contributor

Note I'm cleaning up postgres, the same thing will need to be fixed across dialects likely.

The only actual problem this can cause is that it may prevent CI builds from completing if the CI runner decides that there has been too much output and cancels the build - happens with CircleCi

Contributor

faceleg commented Nov 19, 2015

Note I'm cleaning up postgres, the same thing will need to be fixed across dialects likely.

The only actual problem this can cause is that it may prevent CI builds from completing if the CI runner decides that there has been too much output and cancels the build - happens with CircleCi

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Nov 23, 2015

I'm seeing this warning a lot, not in sequelize directly, but due to how I've set up sequelize with express.

Let's say I have a simple authentication middleware, which looks like this (Session is a sequelize model):

function auth(req, res, next) {
  Session.findById(req.headers.sessionid).then((session) => {
    req.currentSession = session;
    next();
  }).catch(next);
}

Now if I make any sequelize calls later on in the request lifecycle, I get the error:

Warning: a promise was created in a  handler but was not returned from it

(also, for some reason with no stacktrace, which is annoying)

Since further sequelize calls are technically happening inside the next() method, bluebird thinks that I am deliberately nesting sequelize calls, and therefore creating promises inside promises.

I fixed this for now by adding return null right below my next() call, but this seems like a problem that many people will run into, with a fairly non-intuitive solution. I'm not sure whether there is a "more-correct" way to be pre-loading models in express, although I suppose you folks aren't the right people to ask. But is there a way to silence these bluebird warnings altogether for sequelize? They don't seem like a useful feature for library code that I am not working with directly.

(Even if there is nothing which can be done to solve this for now, I figured it was worth writing up my findings for future googlers, since it took a while to find the root cause of this).

I'm seeing this warning a lot, not in sequelize directly, but due to how I've set up sequelize with express.

Let's say I have a simple authentication middleware, which looks like this (Session is a sequelize model):

function auth(req, res, next) {
  Session.findById(req.headers.sessionid).then((session) => {
    req.currentSession = session;
    next();
  }).catch(next);
}

Now if I make any sequelize calls later on in the request lifecycle, I get the error:

Warning: a promise was created in a  handler but was not returned from it

(also, for some reason with no stacktrace, which is annoying)

Since further sequelize calls are technically happening inside the next() method, bluebird thinks that I am deliberately nesting sequelize calls, and therefore creating promises inside promises.

I fixed this for now by adding return null right below my next() call, but this seems like a problem that many people will run into, with a fairly non-intuitive solution. I'm not sure whether there is a "more-correct" way to be pre-loading models in express, although I suppose you folks aren't the right people to ask. But is there a way to silence these bluebird warnings altogether for sequelize? They don't seem like a useful feature for library code that I am not working with directly.

(Even if there is nothing which can be done to solve this for now, I figured it was worth writing up my findings for future googlers, since it took a while to find the root cause of this).

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Nov 23, 2015

Contributor

Personally I think it is a useful feature, especially since it helped me find a bug lurking in my code that I would never have noticed before.

I do agree that the solution is not intuitive and that it is often difficult (read: impossible for mortals) to understand exactly where the **&^ this naughty handler is.

Your example almost exactly matches the example given in the bluebird docs:

getUser().then(function(user) {
    // Perform this in the "background" and don't care about it's result at all
    saveAnalytics(user);
    // return a non-undefined value to signal that we didn't forget to return
    return null;
});

My guess is that the bluebird developer got tired of people "misusing" promises and coming to him with their bugs, and decided to implement this warning to help us all write clearer, easier to debug promise code.

My 2c

Contributor

faceleg commented Nov 23, 2015

Personally I think it is a useful feature, especially since it helped me find a bug lurking in my code that I would never have noticed before.

I do agree that the solution is not intuitive and that it is often difficult (read: impossible for mortals) to understand exactly where the **&^ this naughty handler is.

Your example almost exactly matches the example given in the bluebird docs:

getUser().then(function(user) {
    // Perform this in the "background" and don't care about it's result at all
    saveAnalytics(user);
    // return a non-undefined value to signal that we didn't forget to return
    return null;
});

My guess is that the bluebird developer got tired of people "misusing" promises and coming to him with their bugs, and decided to implement this warning to help us all write clearer, easier to debug promise code.

My 2c

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Nov 23, 2015

I think it would be useful as explicitly opt-in behavior (during testing maybe). I think though that as someone who is fairly new to sequelize (and using es6 promises in my own code), I should really not have to understand peculiarities of bluebird (e.g. inserting return null in random places) to use sequelize.

It would be different of course if I had chosen to use bluebird in my own code, then I would expect possible implementation quirks or to have to read their docs when I see a warning.

Are there any thoughts on moving a future version of sequelize to native es6 promises, and either ship with a polyfill, or ask people to bring their own polyfill if they are using node < v4.0?

I think it would be useful as explicitly opt-in behavior (during testing maybe). I think though that as someone who is fairly new to sequelize (and using es6 promises in my own code), I should really not have to understand peculiarities of bluebird (e.g. inserting return null in random places) to use sequelize.

It would be different of course if I had chosen to use bluebird in my own code, then I would expect possible implementation quirks or to have to read their docs when I see a warning.

Are there any thoughts on moving a future version of sequelize to native es6 promises, and either ship with a polyfill, or ask people to bring their own polyfill if they are using node < v4.0?

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Nov 23, 2015

Contributor

It is probably worth taking the opt-in questions to bluebird as sequelize has no control over what the bluebird devs may or may not include

Contributor

faceleg commented Nov 23, 2015

It is probably worth taking the opt-in questions to bluebird as sequelize has no control over what the bluebird devs may or may not include

@mickhansen

This comment has been minimized.

Show comment
Hide comment
@mickhansen

mickhansen Nov 23, 2015

Contributor

Sequelize currently relies on a great deal of features that native es6 promises don't support.
There are also performance issues to consider (although different benchmarks report different things).

Contributor

mickhansen commented Nov 23, 2015

Sequelize currently relies on a great deal of features that native es6 promises don't support.
There are also performance issues to consider (although different benchmarks report different things).

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Nov 23, 2015

Contributor

My opinion is that the correct approach is to

  1. Fix issues in sequelize highlighted by the new bluebird warnings
  2. Fix issues in userland
  3. Enjoy coding in a more predictable environment.
Contributor

faceleg commented Nov 23, 2015

My opinion is that the correct approach is to

  1. Fix issues in sequelize highlighted by the new bluebird warnings
  2. Fix issues in userland
  3. Enjoy coding in a more predictable environment.
@theptrk

This comment has been minimized.

Show comment
Hide comment
@theptrk

theptrk Nov 26, 2015

@faceleg How are you going about fixing just the postgres dialect? I'm trying to trace the error but it seems like all the promise returns are covered.

theptrk commented Nov 26, 2015

@faceleg How are you going about fixing just the postgres dialect? I'm trying to trace the error but it seems like all the promise returns are covered.

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Nov 27, 2015

Contributor

I've done nothing more than what has been committed here - the fixes in this PR reduced the output such that my builds would pass (CircleCi kills a build if it generates too much output).

The current output is annoying but doesn't break my builds, which drops the priority for me.

Contributor

faceleg commented Nov 27, 2015

I've done nothing more than what has been committed here - the fixes in this PR reduced the output such that my builds would pass (CircleCi kills a build if it generates too much output).

The current output is annoying but doesn't break my builds, which drops the priority for me.

@citium

This comment has been minimized.

Show comment
Hide comment
@citium

citium Nov 27, 2015

Placing return null after /sequelize/lib/dialects/abstract/connection-manager.js:223:16 seem to resolve the disconnect with Warning: a promise was created in a handler but was not returned from it

citium commented Nov 27, 2015

Placing return null after /sequelize/lib/dialects/abstract/connection-manager.js:223:16 seem to resolve the disconnect with Warning: a promise was created in a handler but was not returned from it

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Nov 29, 2015

Contributor

PR! PR! PR please for the love of no warnings

Contributor

faceleg commented Nov 29, 2015

PR! PR! PR please for the love of no warnings

@theptrk

This comment has been minimized.

Show comment
Hide comment
@theptrk

theptrk Nov 29, 2015

Haha, looks like you PR'd it into master so.... soon? Not sure how versioning works.
https://github.com/sequelize/sequelize/blob/master/lib/dialects/abstract/connection-manager.js#L227

theptrk commented Nov 29, 2015

Haha, looks like you PR'd it into master so.... soon? Not sure how versioning works.
https://github.com/sequelize/sequelize/blob/master/lib/dialects/abstract/connection-manager.js#L227

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Nov 30, 2015

Contributor

Oh, right. I know for a fact that I've missed some - all this PR did was stop my CircleCi builds from failing 👍

Contributor

faceleg commented Nov 30, 2015

Oh, right. I know for a fact that I've missed some - all this PR did was stop my CircleCi builds from failing 👍

@mbmccoy

This comment has been minimized.

Show comment
Hide comment
@mbmccoy

mbmccoy Dec 3, 2015

FWIW, this issue is breaking my server tests when I test Sequelize creation following the example. (Mocha aborts due to warnings.)

I can work around this, but I want to emphasize that this issue impacts to my workflow to encourage a fix (and thank you for the work).

mbmccoy commented Dec 3, 2015

FWIW, this issue is breaking my server tests when I test Sequelize creation following the example. (Mocha aborts due to warnings.)

I can work around this, but I want to emphasize that this issue impacts to my workflow to encourage a fix (and thank you for the work).

@theptrk

This comment has been minimized.

Show comment
Hide comment
@theptrk

theptrk Dec 3, 2015

Oh yeah, I've had to move to testing locally.

theptrk commented Dec 3, 2015

Oh yeah, I've had to move to testing locally.

@mickhansen

This comment has been minimized.

Show comment
Hide comment
@mickhansen

mickhansen Dec 4, 2015

Contributor

The latest version has the fixes we've applied so far.
Please provide file locations or PR's with fixes.

Contributor

mickhansen commented Dec 4, 2015

The latest version has the fixes we've applied so far.
Please provide file locations or PR's with fixes.

@joshuakarjala joshuakarjala referenced this issue in sequelize/cli Dec 22, 2015

Closed

Promise warnings #242

@tremby

This comment has been minimized.

Show comment
Hide comment
@tremby

tremby Mar 7, 2016

Contributor

I have one of these too.

Sequelize 3.19.3, using Mysql.

Warning: a promise was created in a handler but was not returned from it
  at [object Object].Instance.save (/home/tremby/myproject/node_modules/sequelize/lib/instance.js:567:18)
  at [object Object].Model.create (/home/tremby/myproject/node_modules/sequelize/lib/model.js:1824:6)
  at [object Object].HasMany.create (/home/tremby/myproject/node_modules/sequelize/lib/associations/has-many.js:533:29)
  at [object Object].obj.(anonymous function) [as createGameSession] (/home/tremby/myproject/node_modules/sequelize/lib/associations/has-many.js:259:24)
  at done (/home/tremby/myproject/src/app/routes/myroute.coffee:43:10)
  at [object Object].<anonymous> (/home/tremby/myproject/src/app/routes/myroute.coffee:58:7)
  at processImmediate [as _immediateCallback] (timers.js:383:17)
From previous event:
...
Contributor

tremby commented Mar 7, 2016

I have one of these too.

Sequelize 3.19.3, using Mysql.

Warning: a promise was created in a handler but was not returned from it
  at [object Object].Instance.save (/home/tremby/myproject/node_modules/sequelize/lib/instance.js:567:18)
  at [object Object].Model.create (/home/tremby/myproject/node_modules/sequelize/lib/model.js:1824:6)
  at [object Object].HasMany.create (/home/tremby/myproject/node_modules/sequelize/lib/associations/has-many.js:533:29)
  at [object Object].obj.(anonymous function) [as createGameSession] (/home/tremby/myproject/node_modules/sequelize/lib/associations/has-many.js:259:24)
  at done (/home/tremby/myproject/src/app/routes/myroute.coffee:43:10)
  at [object Object].<anonymous> (/home/tremby/myproject/src/app/routes/myroute.coffee:58:7)
  at processImmediate [as _immediateCallback] (timers.js:383:17)
From previous event:
...
@Bondifrench

This comment has been minimized.

Show comment
Hide comment
@Bondifrench

Bondifrench Mar 17, 2016

I have got the same problem in Postgres and Sequelize 3.19.3:

Warning: a promise was created in a handler but was not returned from it                                                 
    at processImmediate [as _immediateCallback] (timers.js:383:17)                                                       
From previous event:                                                                                                     
    at Model.findAll (c:\Users\London\Apps\FinviewsApp\node_modules\Sequelize\lib\model.js:1343:18)                      
    at c:\Users\London\Apps\FinviewsApp\routes\api.js:486:16                                                             
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\route.js:100:13)                           
    at Route.dispatch (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\route.js:81:3)                   
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:234:24                                  
    at Function.proto.process_params (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:312:12)  
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:228:12                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:191:16                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:191:16                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:191:16                                  
From previous event:                                                                                                     
    at Promise.then (c:\Users\London\Apps\FinviewsApp\node_modules\Sequelize\lib\promise.js:21:17)                       
    at c:\Users\London\Apps\FinviewsApp\config\passport.js:21:5                                                          
    at pass (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\authenticator.js:353:9)                          
    at Authenticator.deserializeUser (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\authenticator.js:358:5) 
    at SessionStrategy.authenticate (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\strategies\session.js:49:
    at attempt (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\middleware\authenticate.js:341:16)            
    at authenticate (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\middleware\authenticate.js:342:7)        
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at trim_prefix (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:270:13)                    
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:237:9                                   
    at Function.proto.process_params (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:312:12)  
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:228:12                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at initialize (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\middleware\initialize.js:62:5)             
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at trim_prefix (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:270:13)                    
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:237:9                                   
    at Function.proto.process_params (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:312:12)  

I have got the same problem in Postgres and Sequelize 3.19.3:

Warning: a promise was created in a handler but was not returned from it                                                 
    at processImmediate [as _immediateCallback] (timers.js:383:17)                                                       
From previous event:                                                                                                     
    at Model.findAll (c:\Users\London\Apps\FinviewsApp\node_modules\Sequelize\lib\model.js:1343:18)                      
    at c:\Users\London\Apps\FinviewsApp\routes\api.js:486:16                                                             
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\route.js:100:13)                           
    at Route.dispatch (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\route.js:81:3)                   
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:234:24                                  
    at Function.proto.process_params (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:312:12)  
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:228:12                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:191:16                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:191:16                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:191:16                                  
From previous event:                                                                                                     
    at Promise.then (c:\Users\London\Apps\FinviewsApp\node_modules\Sequelize\lib\promise.js:21:17)                       
    at c:\Users\London\Apps\FinviewsApp\config\passport.js:21:5                                                          
    at pass (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\authenticator.js:353:9)                          
    at Authenticator.deserializeUser (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\authenticator.js:358:5) 
    at SessionStrategy.authenticate (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\strategies\session.js:49:
    at attempt (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\middleware\authenticate.js:341:16)            
    at authenticate (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\middleware\authenticate.js:342:7)        
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at trim_prefix (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:270:13)                    
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:237:9                                   
    at Function.proto.process_params (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:312:12)  
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:228:12                                  
    at Function.match_layer (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:295:3)            
    at next (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:189:10)                           
    at initialize (c:\Users\London\Apps\FinviewsApp\node_modules\passport\lib\middleware\initialize.js:62:5)             
    at Layer.handle [as handle_request] (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\layer.js:76:5) 
    at trim_prefix (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:270:13)                    
    at c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:237:9                                   
    at Function.proto.process_params (c:\Users\London\Apps\FinviewsApp\node_modules\express\lib\router\index.js:312:12)  
@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Mar 17, 2016

Contributor

Cursory glances through https://github.com/sequelize/sequelize/blob/master/lib/model.js#L1345 don't reveal a reason.

Are you sure you're returning from any then you're using in your code?

Contributor

faceleg commented Mar 17, 2016

Cursory glances through https://github.com/sequelize/sequelize/blob/master/lib/model.js#L1345 don't reveal a reason.

Are you sure you're returning from any then you're using in your code?

@tremby

This comment has been minimized.

Show comment
Hide comment
@tremby

tremby Mar 17, 2016

Contributor

Not sure if you're talking to one or the other of us, or both, but I'm afraid I have no recollection what I was doing at the time to get the message I did, so I'll bow out of this.

Contributor

tremby commented Mar 17, 2016

Not sure if you're talking to one or the other of us, or both, but I'm afraid I have no recollection what I was doing at the time to get the message I did, so I'll bow out of this.

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Mar 17, 2016

Contributor

@mickhansen recommend this thread is locked with a comment sending people to the Bluebird 3.0 docs regarding this new warning.

Contributor

faceleg commented Mar 17, 2016

@mickhansen recommend this thread is locked with a comment sending people to the Bluebird 3.0 docs regarding this new warning.

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Mar 17, 2016

@faceleg agree that it's turning into a bit of a general support thread, but I still view this as something that should be fixed in sequelize, because an implementation detail is exposing hard-to-diagnose errors which we would not otherwise see (and has forced me to put pointless return null statements in my code to avoid it).

I think it would be best if sequelize could either disable this error, or not depend on a promise library which throws warnings for perfectly valid code.

@faceleg agree that it's turning into a bit of a general support thread, but I still view this as something that should be fixed in sequelize, because an implementation detail is exposing hard-to-diagnose errors which we would not otherwise see (and has forced me to put pointless return null statements in my code to avoid it).

I think it would be best if sequelize could either disable this error, or not depend on a promise library which throws warnings for perfectly valid code.

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Mar 17, 2016

Contributor

I disagree. The warning was added to Bluebird to save us from wasting time debugging issues caused when we forget to return from a promise.

If Bluebird adds an option to disable it, it should be up to the end-developers to decide if they want it off.

We have done our best to resolve this issue in sequelize, if a programmer using sequelize is getting this warning, it is very likely that the issue is with their code, not this library.

Contributor

faceleg commented Mar 17, 2016

I disagree. The warning was added to Bluebird to save us from wasting time debugging issues caused when we forget to return from a promise.

If Bluebird adds an option to disable it, it should be up to the end-developers to decide if they want it off.

We have done our best to resolve this issue in sequelize, if a programmer using sequelize is getting this warning, it is very likely that the issue is with their code, not this library.

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Mar 17, 2016

Right, but as my example further up pointed out, this warning is triggered very easily if you ever use Sequelize inside an express middleware (very common scenario). In this case it's not programmer error or an issue with their code, and many people using Sequelize may not even be aware that it's using Bluebird underneath. Hence why they are googling the error and landing here.

If we're not going to disable the warnings, then it might be worth at least adding a page to the Sequelize docs explaining that using Sequelize inside express middleware will cause Bluebird warnings, and explain how to work around it. Just forwarding people to the Bluebird docs will only lead to more confusion.

Right, but as my example further up pointed out, this warning is triggered very easily if you ever use Sequelize inside an express middleware (very common scenario). In this case it's not programmer error or an issue with their code, and many people using Sequelize may not even be aware that it's using Bluebird underneath. Hence why they are googling the error and landing here.

If we're not going to disable the warnings, then it might be worth at least adding a page to the Sequelize docs explaining that using Sequelize inside express middleware will cause Bluebird warnings, and explain how to work around it. Just forwarding people to the Bluebird docs will only lead to more confusion.

@faceleg

This comment has been minimized.

Show comment
Hide comment
@faceleg

faceleg Mar 17, 2016

Contributor

using Sequelize inside express middleware will cause Bluebird warnings

Not returning null from the final .then callback is what will cause warnings, nothing to do with middleware.

many people using Sequelize may not even be aware that it's using Bluebird underneath

When incorporating libs into code that is intended to be used in production, it is my opinion that it is the responsibility of the lead developer to ensure they understand at least the first-level dependencies of said libs.

Agree the docs could be updated to point to http://bluebirdjs.com/docs/warning-explanations.html#warning-a-promise-was-created-in-a-handler-but-none-were-returned-from-it

Also the no stack trace was a bug and should have been fixed: petkaantonov/bluebird#846

Contributor

faceleg commented Mar 17, 2016

using Sequelize inside express middleware will cause Bluebird warnings

Not returning null from the final .then callback is what will cause warnings, nothing to do with middleware.

many people using Sequelize may not even be aware that it's using Bluebird underneath

When incorporating libs into code that is intended to be used in production, it is my opinion that it is the responsibility of the lead developer to ensure they understand at least the first-level dependencies of said libs.

Agree the docs could be updated to point to http://bluebirdjs.com/docs/warning-explanations.html#warning-a-promise-was-created-in-a-handler-but-none-were-returned-from-it

Also the no stack trace was a bug and should have been fixed: petkaantonov/bluebird#846

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Mar 17, 2016

Not returning null from the final .then callback is what will cause warnings, nothing to do with middleware.

Sure, but that's not something I would normally do inside express middleware.

Honestly it doesn't sound like I'm going to be able to convince you that this is confusing and hard to debug for people. I think this is a leaky abstraction - people shouldn't need to spend time understanding implementation details of libraries before they can use them. Even that page you linked to does nothing to explain why Bluebird would interact badly with non-promise aware libraries like express.


Anyway, in case it gets locked, hopefully there is enough detail in this thread to help future googlers:

All of the warnings being generated from within Sequelize have now been fixed. To avoid the error, try just putting return null anywhere you are using Sequelize promises (especially when mixing them with callbacks):

Before:

function auth(req, res, next) {
  Session.findById(req.headers.sessionid).then((session) => {
    req.currentSession = session;
    next();
  }).catch(next);
}

After:

function auth(req, res, next) {
  Session.findById(req.headers.sessionid).then((session) => {
    req.currentSession = session;
    next();
    return null;
  }).catch(next);
}

Not returning null from the final .then callback is what will cause warnings, nothing to do with middleware.

Sure, but that's not something I would normally do inside express middleware.

Honestly it doesn't sound like I'm going to be able to convince you that this is confusing and hard to debug for people. I think this is a leaky abstraction - people shouldn't need to spend time understanding implementation details of libraries before they can use them. Even that page you linked to does nothing to explain why Bluebird would interact badly with non-promise aware libraries like express.


Anyway, in case it gets locked, hopefully there is enough detail in this thread to help future googlers:

All of the warnings being generated from within Sequelize have now been fixed. To avoid the error, try just putting return null anywhere you are using Sequelize promises (especially when mixing them with callbacks):

Before:

function auth(req, res, next) {
  Session.findById(req.headers.sessionid).then((session) => {
    req.currentSession = session;
    next();
  }).catch(next);
}

After:

function auth(req, res, next) {
  Session.findById(req.headers.sessionid).then((session) => {
    req.currentSession = session;
    next();
    return null;
  }).catch(next);
}
@mickhansen

This comment has been minimized.

Show comment
Hide comment
@mickhansen

mickhansen Mar 18, 2016

Contributor

@amacneil I agree it's hard to debug, but disabling the warnings is out of our control.
We'll have to make an effort to get the "answer" high up there when people search for it.

Contributor

mickhansen commented Mar 18, 2016

@amacneil I agree it's hard to debug, but disabling the warnings is out of our control.
We'll have to make an effort to get the "answer" high up there when people search for it.

@mickhansen

This comment has been minimized.

Show comment
Hide comment
@mickhansen

mickhansen Mar 18, 2016

Contributor

Environment variables for disabling warnings: http://bluebirdjs.com/docs/api/environment-variables.html

Contributor

mickhansen commented Mar 18, 2016

Environment variables for disabling warnings: http://bluebirdjs.com/docs/api/environment-variables.html

@sequelize sequelize locked and limited conversation to collaborators Mar 18, 2016

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