@alubbe bunch of tests on travis fail, sure you ran all tests?

hmm that is weird.
I cleaned up my repo and npm caches, and am now getting the same errors. Sorry about that!

A/B testing showed that the problems started with bluebird 2.4.0. Looking at the the changelog, I don't really see what is breaking all of these tests: https://github.com/petkaantonov/bluebird/blob/master/changelog.md#240-2014-12-18

It seems to affect .all - any idea of what's going on?

For reference, this is one of the lines that are now failing: https://github.com/sequelize/sequelize/blob/master/test/associations/has-many.test.js#L1458

maybe it is in the .on listeners?

Some internal changes are described in the changelog, that might interfer with out backwards compat monkey patching.

Yes, the monkey patching broke during two bluebird commits. I have identified both but only managed to fix the first instance (see above).

The other breaking change occured here: petkaantonov/bluebird@8c1edaf#diff-bdbe8b178ed02fc38d7fda842fc1975aL297

See this discussion: promises-aplus/promises-spec#179

It would be quite useful to use that newer version of bluebird to avoid running into this kind of memory leak. However, bluebird now 'proxies' its promises so that the 'sql' event listeners are hooked up to the wrong objects. I have spent 5 hours trying to figure it out and have given up. Maybe someone with a deeper knowledge of the monkey patch can take a look?

Getting the performance bump would definitely be usefull.
Ideally we will remove the monkey patching in time, we have to figure out what we want to do with on('sql') though which is quite helpfull in debugging.

Ideally we will remove the monkey patching in time, we have to figure out what we want to do with on('sql') though which is quite helpfull in debugging.

Could you transport sql on promise context?

@cusspvz not sure what you mean by that, there isn't a promise context per say as far as i know.

Model.findAll().then(function() { console.log( this.sql ) })

And you should have a minor performance increase (related with event emitting) since Javascript V8 deletes it from memory if there aren't references to bounded promise's context on promise's handlers, before running them.

@cusspvz Ah i see, although that doesn't exactly do what on('sql') did, (you could get the entire promise chain sql calls with just one event listener.

And what if context.sql was an array with all of them?
The only difference is that it won't be called before each sql execution.

@cusspvz That's could be a possibility, but how would you attach something to the promise context? Afaik that's not possible, especially not if people using .bind(), and .bind() contexts dont transfer from chain to chain.

Since dialect/query.js logs SQL with sequelize.log method and promise.bind() isn't a good approach, I don't see a point to spend resources like CLS for providing more debugging possibilities.

I would vote to default logging to require( 'debug' )( 'sequelize' ) and let users choose to view SQL queries trough DEBUG=sequelize /* OR */ DEBUG=* environment variable.

@cusspvz sequelize.log is horrible for debugging individual calls. So would DEBUG be.
But i generally agree with you, maybe make it possible to pass individual log or logging options to calls for individual debugging.

If bluebird had the support for progress, sql could be passed as a progress stage.

I meant to default Sequelize constructor logging option to use debug npm module instead of console.log (console.log is really uggly for debugging), but I liked your idea of passing individual logging method as queryOption.

@mickhansen removing the monkey patching would be ideal, specially because io.js (the node fork) is going to ship native spec-compliant ES6 Promises on its first release 1.0.0 (tomorrow) without requiring any kind of runtime flag. Node 0.11.13+ also supports them when using --harmony.

This means that in order to ensure future compatibility, you should look into replacing require('bluebird') by require('native-or-bluebird') (see native-or-bluebird) and make sure the code is compatible with both versions. This should be trivial (1:1) if no custom patches are applied.

Also, progress is not part of ES6 Promises, so I'd avoid that. I can experiment a bit with ES6 Promises on sequelize, but you're probably the right person :)

@ruimarinho removing the monkey patching is not an option untill atleast a few versions into 2.0.
We don't use progress, we use all, join, map, then, catch etc though - I don't think they are all covered by native promises but we shall have to check.

I assume native-or-bluebird is also compatible with node 0.8?
We haven't started running tests against neither 0.11 or io.js (i know MariaDB does not work with 0.11)

@ruimarinho native promises currently consume more memory, are significantly slower and are harder to debug than bluebird promises - they also expose a much less rich API. I don't understand why anyone would rather use them on the server (client is another story)

@cusspvz technically progress reliant code will still run - but it will break eventually and was deprecated and for a good reason. Progression on promises never really worked very well - you can read the API docs about the migration from progression to lack of. I can also dig kriskowal's notes on why it didn't work and what he's doing instead in Q 2.0 . It's worth mentioning that native promises don't support it and likely never will.

@benjamingr Thanks for the notice, i wasn't aware of that. :)

Okay so while we can't upgrade bluebird all the way, we could merge this PR to get to 2.4.2 and enjoy the reduced memory usage.

Also, who would be the person here to contact/brainstorm with in order to update the monkey patch to newer versions of bluebird ?

@alubbe https://github.com/petkaantonov/bluebird/blob/master/changelog.md memory usage improvement is in 2.6

But yes we might as well merge the upgrade :)

@alubbe @janmeier and i wrote most of the monkey patching, but he might be the most familiar by now.

@mickhansen https://github.com/petkaantonov/bluebird/blob/master/changelog.md#240-2014-12-18 Promises now delete references to handlers attached to them as soon as possible means faster garbage collection - but 2.6 is indeed the biggie here

@alubbe ah yes you are right :)

@mickhansen @janmeier
Looking at both the changes in bluebird and the EventEmitter of iojs and the fact that the "sql" event is mostly used for debugging and sequelize's own tests, I think the best way forward would be to think of a different and more compatible implementation. What are the reasons you want to put this off until 2.2? 2.0 will already be backwards-incompatible, so why not save yourselves the hassle of maintaining a monkey patch for another 6-12 months (or however long away 2.2 is)?

One way that you could do it is to use the $sql property directly, instead of emitting it. So, for example

var promise = User.findAll(...).then(function(users) {
  console.log(promise.$sql); //available here after db's callback
}
console.log(promise.$sql); // ... and here during the same stack and servertick

So you can just extend SequelizePromise from bluebird's Promise and be done. The tests can be changed to checking for the length of the array instead of the number of times that spy has been called/the event was emitted.

What do you think?

@alubbe eventemitter was made deprecated in 2.0, not removing it yet.

Your proposal for $sql makes sense if it works.
Ideally we want to remove that completely in the future though, perhaps just support logging or debug statements on each call or similar.

Generally all the tests that test on SQL should be moved to unit tests instead of integration tests.

okay I find that even better. What exactly is the reason for keeping it in 2.0 (and 2.1) though?

@alubbe It's a major syntax change for implementers, we want to give them a warning for a version atleast before removing it completely :)

Can I convince you to drop it with 2.1? :D

@alubbe let's see :)

Well it's for the reasons I outlined above. Monkey-patching is a lot of work to maintain - bluebird is now on 2.8 and eventemitter is also going through changes. I think most people in this thread agree that the sql event isn't really that useful and can be replaced by using logging or storing it in $sql, which would make the monkey-patching unnecessary.

The time that has passed since 1.7 is pretty big, so I am somewhat worried that 2.2 will be pretty far off. And I think it is not unreasonable to expect that every few months something else will break or change that will cost maintenance for this promise re-implementation. So I believe that the sooner it can be removed the more up2date the dependencies will be and ultimately the better sequelize will be.

@alubbe First step would be to provide an adequate solution for call-level debugging. 1.7 to 2.0 was a long time because we had to remove so much cruft for the better, should move faster after 2.0 final :)

If we get a good solution for call level debugging i could be inclined to remove the event emitter patch for 2.1

Is there some place where this is currently discussed?

I believe so but i couldn't find it with a quick search.

do let me know when you find it and I'll share my two cents

Well if i trip over it i will :)
I think the solution will likely be something like logging: true||console.log passed to queryOptions for calls.

I agree. What besides logging/writing to stdout was the sql event used for? Are we losing anything by not passing the string around?

It was just easier to debug a single call in an entire chain, or debug an entire chain vs an entire application. Just localized debugging, which is nice :)

but telling the call to log via queryOptions does exactly that, right? So there were no use cases apart from integration tests for passing back the string rather than just printing it out?

@alubbe that only works for a single call, no way to debug a chain, but yeah it's probably the best solution

The only use cases are debugging and working with the sql.
Checking the SQL does not belong in the integration tests ideally, we are creating a new suite of unit tests for that

can you give me an example of a chain where this wouldn't work?

@alubbe well its just if you have a series of calls, you need to attach logging to each, where previously you could do on('sql') on the wrapping promise to get the sql events for everything inside.

But it's not so important that it's stopping the change, it's just a neat tool.

Is there some place where this is currently discussed?

I believe so but i couldn't find it with a quick search.

On this PR - #2882 (comment)

@alubbe also we need to change all tests to use promises :(

We don't use progress, we use all, join, map, then, catch etc though - I don't think they are all covered by native promises but we shall have to check.

Beside then and catch, native methods support by ES6 Promises are .all, .race, .reject and .resolve.

I assume native-or-bluebird is also compatible with node 0.8?

@mickhansen, it just falls back to bluebird if the global Promise object is not available, so it is as supported as bluebird gets.

@benjamingr you're right that they consume more memory and are about 4x slower than bluebird, but if you look at the big picture, you can't possible optimise bluebird more than what the V8 will be able to do at the VM layer. It's simply a matter of time. So if we can prepare sequelize or any other lib for that matter to be compatible with ES6 Promises, even if that means using bluebird for now (just stick with node in order to be left outside this feature), it would be a huge step.

Besides the custom monkey patching for logging, my bet is most things can be adapted with only a small number of changes (removing usage of join and map).

@mickhansen if removing event emitter support is a BC as big as it sounds, I couldn't possible think of another better time to do that than 2.0.0. So much time has passed that I suspect most people interested in upgrading have already experimented with RCs at least.

@ruimarinho problem is we're releasing 2.0 in a few days - And the change has only been made deprecated for 2.0 - I'd like to have it deprecated in a release for a version or two before actually removing it.

Also all the tests still need to be converted to promise based.