Change approach to how this patch works internally

[overlookmotel]

I've been working on a new set of tests. Here it is (work in progress): https://github.com/overlookmotel/cls-bluebird/tree/tests-rewrite

And the test results on Travis: https://travis-ci.org/overlookmotel/cls-bluebird/jobs/140228913

As you can see, it's getting a bit out of control! There are so many different combinations of:

• methods
• promises resolved synchronously or asynchronously
• promises created and methods called in different CLS contexts
• promises resolved from a variety of other thenables

There's already 250 tests, and that's purely for Promise.resolve() and Promise.reject() with bluebird v2 - haven't written tests for any of the other methods yet! I'm also not sure that I've covered every possible case.

I've come to the conclusion that this is the wrong way to go about it.

The problem

The current patch simply patches internal bluebird method promise.prototype._addCallbacks to bind callbacks to the current CLS namespace. The logic is that every bluebird method that adds a callback to a promise calls _addCallbacks internally, so the callbacks are always bound.

This is really smart and elegant, but it relies on the above assumption.

In bluebird v3 the internals of bluebird changed so that _addCallbacks is bypassed when a calling .then() on a promise that's already resolved. So that assumption was no longer correct, and it broke the cls-bluebird patch.

I've also just found a bug with Promise.reduce() in bluebird v2 (issue #12) - again, Promise.reduce() doesn't call _addCallbacks.

A proposal

I suggest that a better approach would be to patch each of bluebird's public methods individually, much like Sequelize's bluebird/CLS shim does.

Why?

This is much less clever and not so elegant, but it does have the following advantages:

1. Internal changes to bluebird in future versions would not break the patch (NB internal changes that don't affect the public API could happen in even a patch release).
2. More robust - less likely that there's weird edge cases where whatever assumptions the patch is based on aren't correct.
3. Tests can be much simpler and cover all cases with greater reliability.

The tests would only have to cover two areas:

1. Every public method binds all callbacks which are executed asynchronously.
2. Every public method returns a promise which is an instance of the patched bluebird constructor (i.e. Bluebird.resolve( Q.resolve() ) returns a Bluebird promise not a Q promise).

I think this would make it possible to write a set of tests that you can be confident really do cover all cases.

What's the downside?

It's possible that some callbacks will be bound unnecessarily.

An example: Promise.map( arr, fn ) executes fn synchronously if arr is an array of literal values. If it's an array of promises, however, fn is executed async after the promises resolve.

To cover the latter case, fn would have to be bound, but that's unnecessary when it ends up being called synchronously. So it's a small performance hit.

Why does this matter so much anyway?

My feeling is for CLS "mostly works" isn't good enough. You need to have 100% confidence that it's completely bullet-proof.

As @iliakan said in othiym23/node-continuation-local-storage#59, the consequences of CLS contexts getting mixed up between requests on an HTTP server can be quite dire e.g. a regular user getting admin privileges.

So, in my opinion, even if my proposal is a lot less "nice" and also slightly less performant, it's worth it for the increased reliability.

@TimBeyer what do you think?

[overlookmotel]

It'll look something like this: https://github.com/overlookmotel/cls-bluebird/tree/refactor/lib

[Jeff-Lewis]

So, in my opinion, even if my proposal is a lot less "nice" and also slightly less performant, it's worth it for the increased reliability.

IMHO - I would expect most projects that are needing cls-bluebird, need 100% CLS reliability. I also imagine the performance is negligible and could be mitigated by changing the promise collection strategies.
@overlookmotel awesome job on this. I'm excited to test it out.

[overlookmotel]

Thanks @Jeff-Lewis.

The repo https://github.com/overlookmotel/cls-bluebird/tree/refactor is updated with my latest. I think it's now all working and covering all methods for both bluebird v2.x and v3.x.

Now am working on the tests. They should bring up anything I've missed.

[overlookmotel]

What did you mean "mitigated by changing the promise collection strategies"?

If you mean patching Promise.map() differently, I've had a go at that. See https://github.com/overlookmotel/cls-bluebird/blob/refactor/lib/shimMapBluebird3.js if you're interested.

Actually, I'm not sure that the behavior of Promise.map() in bluebird v3 is how it should be. It looks a bit like Zalgo to me. Have raised an issue for it petkaantonov/bluebird#1148.

[overlookmotel]

@TimBeyer If you have time, would you be able to say if you're happy with the direction I'm taking with this?

[overlookmotel]

Brief update: I'm getting on quite well with this. Have finished a big suite of tests for the core methods and they're all passing. Next up tests for the collection methods...

Working copy is here: https://github.com/overlookmotel/cls-bluebird/tree/refactor

[overlookmotel]

New version is finished. Just submitted it as PR #15.