-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add continuation flow doc #94
Conversation
d372147
to
b4052ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up! I've been meaning to find the time to put something together myself but have been very distracted recently by other things...
I also feel the set/get semantics discussion interconnects in a lot of ways that just make both flow semantics a whole lot more usable, so I want to write something explaining in detail how those relate.
In any case, I've left a few comments on how I see context merging working, though I want to express again that merges are far less important than just being able to link back to any directly originating context, flattening everything up to the root makes for quite unhelpful traces. 😐
model, modifications made in an async subtask are propagated to its | ||
continuation tasks. | ||
|
||
It was initially proposed based on the callback style of async continuations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this is how AsyncContext as it is proposed now will work with callbacks. It's only promise and async/await behaviour that is the outlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this section is trying to demontrate the reason behind the continuation flow.
CONTINUATION.md
Outdated
- For `ContinuationFlow`, the proposed solution didn't specify | ||
conflicts-resolving yet. If the default behavior merges the values in an | ||
array, it changes the shape of the value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can both pick a winner and provide an aggregate. Imagine something like this:
const aggregate = new AggregateContext()
await Promise.all([
aggregate.add(randomTask()),
aggregate.add(randomTask()),
aggregate.add(randomTask()),
])
The AggregateContext
can expose the first value as the current context (or whichever one finishes last, which is the current semantic of ALS) but then the context API could additionally have something like store.isAggregate
to indicate if there are multiple values being held in aggregate and then one could do store.getAggregate()
instead to get that differently shaped context set. Now imagine if that use of AggregateContext
above just happened implicitly on all the promise-merging functions.
That's basically how I see it being possible to support single-shape semantics while also still retaining the full causal graph. It's not really so important that we retain all of them so much as that we just have at least one of them as otherwise most apps would just flatten all activity up to the top-level. It's okay if a branch goes off to nowhere, but it's not okay if subsequent execution skips over all originating activity and just links back to the root of the trace--that makes for a rather useless picture of what the application is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind expanding on "whichever one finishes last, which is the current semantic of ALS"? Like in the following example:
const { AsyncLocalStorage } = require('async_hooks')
const als = new AsyncLocalStorage()
function randomTask(value) {
return als.run(value, () => {
return new Promise(resolve => {
setTimeout(() => {
resolve()
}, Math.random() * 1000)
})
})
}
als.run('main', async () => {
await Promise.all([
randomTask(1),
randomTask(2),
randomTask(3),
])
console.log(als.getStore())
})
It currently prints 'main'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, miscommunication. I meant if applying the continuation flow this is how AsyncLocalStorage would behave given the current execution model. Each promise resolution would replace the context at the point at which that branch completes, so the last one to complete would be the final state when the aggregate promise resolves.
CONTINUATION.md
Outdated
|
||
- For `AsyncContext.Variable`, it is `main`, | ||
- For `ContinuationFlow`, it is the one caused the rejection, and discarding | ||
leaf contexts of promises that may have been fulfilled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a case where I'm more a fan of something like error.originatingContext
so it can be directly tied to the error itself and not intermediate machinery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a property like error.originatingContext
that can be represented with langugage values should be preferred over depending on implicit syntax structures. In this sense, what would you expect the current surrounding context would be for continuation flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you described, the "flow" is reaching the point of rejection and then aborting the aggregate, so that would be the context I would expect. But layered promises would produce something similar to stack unwinding where we would lose context of the exact source, which is why I feel an error.originatingContext
is a more useful source for the particular purpose of error analysis.
CONTINUATION.md
Outdated
For the two type of context variables, the value at site (2): | ||
|
||
- For `AsyncContext.Variable`, the context is `'init'`. | ||
- For `ContinuationFlow`, the context is still `'resolve'`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth stating that if onFulfill
was set here then the 'resolve'
context should flow into that and then through it. Meaning if onFulfill
changed the context in a nested promise it'd return that new value if you then followed with another then(...)
that internally fetched the context value.
CONTINUATION.md
Outdated
|
||
### Fulfilled promises | ||
|
||
Similar to the rejected promise issue, the MOST relevant cause's context when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should abandon the idea of "most relevant cause", since that leads to problematic heuristics. Instead, the goal should be that the winning context is clearly defined, preferably with as simple/straightforward a ruleset as possible. I think this is one point in AsyncContext.Variable
's favor, since "the context is snapshotted the moment any function to schedule is passed to a builtin" is very clearly specified with no ambiguity, whereas any other context is going to be much more situational.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this section, it is showing the example where the most relevant cause is not observable from JavaScript at the moment and why it could be undeterministic. I rephrased the section to highlight the intention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the context is snapshotted the moment any function to schedule is passed to a builtin" is not strictly correct for what exists when considering async/await, which is exactly what this is all about. A snapshot is captured when an await expression is made. It has nothing to do with any function being passed to something as you could await anything.
That aside though, capture and restore points are consistent with through flow semantics, and restore points are actually the same between the two. The only difference is that snapshot captures at the end of a branch rather than the start. In the case of a singular branch and merge, the execution is linear so the expected behaviour is fairly obvious that the context flows forward through the branch and out the other end. In the case of multiple branches merging such as with Promise.all(...)
there are some additional complexities, but a winner can be picked with no special handling in that the resolution of the aggregate promise would occur as a direct result of the final inner branch completion and so would just adopt that context automatically unless we did something else to change that behaviour.
We could have merge contexts to express multiple context origins, but it would still be entirely possible to "pick a winner" to at least be the default value, which seems like a reasonable trade-off. The matter of if we should even have a merge context can be a separate issue, and is certainly lower-priority than the base flow-through semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Document the discussed semantics of continuation flow.
/cc @Qard please feel free to suggest correction of the semantics since many of the details were not expanded.
Closes #83