-
Notifications
You must be signed in to change notification settings - Fork 207
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
Continuations do not work across callbacks unless wrapped with namespace.run() #66
Comments
@parasyte interesting find! How does it do with cls's tests? |
@Jeff-Lewis I was super lazy and didn't run the test suite last night... I'm getting 5 test failures on master (just after clone, not patched yet) node 6.2.1, OSX 10.11.5: https://gist.github.com/parasyte/78792c38d628b2ef619c3041871e2b3b Hmm, same failures on Ubuntu 14.04 and node 4.4.5 ... Well, anyway... There is one new failure with the patch:
So process.nextTick(function () {
t.equal(writer.active.value, outer.value, "writer.active.value == outer.value");
}); Give me a bit and I'll submit a PR with tests. |
@parasyte In my opinion, your first example is the desired behavior. CLS guarantees that once a context is bound (with
Probably the most common use case for CLS is setting a unique ID for each request received by an HTTP server. So then e.g. any In such cases, the If you want to fork the context, you can always do so with a 2nd CLS is on a very hot code path. I expect that the performance impact of creating a new context on every single callback could be pretty bad. |
I think this would be a more common example of kind of structure that CLS supports, and demonstrates it's usefulness (note the addition of 2nd "use strict";
const cls = require('continuation-local-storage');
let ns = cls.createNamespace('test');
const tee = "\u251c\u2500\u2500";
const angle = "\u2514\u2500\u2500";
const pipe = "\u2502";
let log = [];
function output(log) {
log.forEach(function (line) {
console.log(line);
});
}
ns.run(function () {
ns.set('value', 0);
log[0] = `start: -> ${ns.get('value')}`;
setTimeout(function () {
let first = ns.get('value');
ns.set('value', 1);
log[1] = `${tee} first: ${first} -> ${ns.get('value')}`;
setTimeout(function () {
let third = ns.get('value');
ns.set('value', 3);
log[2] = `${pipe} ${angle} third: ${third} -> ${ns.get('value')}`;
}, 200);
}, 100);
});
ns.run(function () {
ns.set('value', 0);
setTimeout(function () {
let second = ns.get('value');
ns.set('value', 2);
log[3] = `${angle} second: ${second} -> ${ns.get('value')}`;
setTimeout(function () {
let fourth = ns.get('value');
ns.set('value', 4);
log[4] = ` ${angle} fourth: ${fourth} -> ${ns.get('value')}`;
output(log);
}, 200);
}, 200);
}); The two blocks inside This would produce your desired output:
NB this is better than you'd get from the mock you provided. Am I making any sense? This stuff is quite hard to get your head around. And when you try to reason how it should behave with promises... ahahahahaaa! |
Ah! Thanks @overlookmotel, that makes perfect sense. I misunderstood the subtlety of where As for how to reason about CLS behavior with promises, that's easy! "Convention 1" that you described in #64 is the obvious expected behavior. To quote one of the Promises/A+ authors:
Convention 1 is the only one of the three which behaves as a transformation. But I digress! Thanks again for clearing up my understanding of CLS! This ticket (and the linked PR) are irrelevant. |
No worries. Glad I could help. Concerning your opinion on which is the right convention for cls/promises, would you mind making your views known in a comment on #64? It'd be good to get some discussion going and/or people's views to be there for anyone who comes across the thread later on. |
At first glance, the docs indicated this module is exactly what I've been looking for!
So I went digging through the code to get an understanding of the black magic that makes this work. Looks like it just relies on closures and a stack ... But closures are totally useless across callbacks, so there must be something more tricky going on. I see
async-listener
is used, and its documentation is pretty clear; this is the witchcraft that does the stuff!Let's validate:
With this, I should expect four callbacks to run, each about 100ms apart. And I should expect the output to be:
This would be proof that the context really does retain its parent's value.
Run it and ... NOPE! 😢 Actual output is:
The context is not retaining its parent's value at all; it is retaining the value from the last assignment. This is no better than replacing the
ns
reference with a mock:(This mock "namespace" object will return the same output.)
But what about that
ns.run()
method? I only use it once to initialize the context ... Let's see what happens when I wrap all my callbacks with it!😳 It works! It's exactly what I need... kind of ugly, though. But what kind of voodoo is this... ?!
... Oh, it's the stack! Of course. So in the first example, the context isn't being pushed to the stack when callbacks are created. But I thought that was the whole purpose of using
async-listener
in the first place?Welp:
node-continuation-local-storage/context.js
Line 168 in 4b5cfc4
Let's patch it up to push the context for each callback created:
Run the first example with the patch and ... problem solved! 🎉
One more test just to ensure the nested functions aren't doing something funny ... I should be able to use this patch with functions defined in a separate scope and it should still work the same:
The patch even works with this variant, so
async-listener
is doing its job at thesetTimeout
calls, not on function declaration. perfectThe text was updated successfully, but these errors were encountered: