12:47 | <Stephen Belanger> | For promise-based APIs: I am having trouble picturing what we would want and how; maybe you could give a concrete example of where you don’t want the restore-around-await semantics (“registration time”) and what you want instead? |
12:53 | <Stephen Belanger> | So our main concern for APM uses is that code which is functionally equivalent from the user perspective should produce an equivalent context graph. In the example I posted yesterday (https://gist.github.com/Qard/6ceaca8bb792679e82c7693513baee0e) I have three examples of an http request using async/await, raw promises, and callbacks respectively. All three examples are functionally identical from user-facing execution flow perspective, yet they produce very different context graphs from AsyncContext because promises and async/await follow registration path rather than resolve path. |
12:54 | <Stephen Belanger> | It's very confusing to users when they rewrite their code from callbacks to promises and suddenly their traces look very different because the flow of the context graph is so different. |
14:10 | <littledan> | It is hard to square wanting to never restore around await with what you wrote in the doc about how AsyncLocalStorage mostly does what you want (when it restores around await) |
14:11 | <littledan> | I was persuaded by your doc that both paths are meaningful but now it seems like you are saying only one of them is, which is surprising |
14:14 | <littledan> | One suboptimal way to implement promise.all is to go for-of through the array and await each element. In this case, we would care about establishing links both from the context before the await, and from the thing we are awaiting |
14:14 | <littledan> | (Assuming the context before the await somehow inherits the previous thing being awaited) |
14:16 | <littledan> | I see how, in our web integration, we could/should adopt the “prefer originating context when present” semantics. But it is still hard for me to understand this await argument. |
14:19 | <Andreu Botella> | This wouldn't be the case, unless we somehow make the calling context be an array of the contexts of all promises that led to this point, which seems like clear overkill |
15:07 | <Stephen Belanger> | It is hard to square wanting to never restore around await with what you wrote in the doc about how AsyncLocalStorage mostly does what you want (when it restores around await) |
15:07 | <Stephen Belanger> | ALS is "good enough" that we can provide a product that "works" but is not good enough to provide something that works well. |
15:08 | <Stephen Belanger> | This is the whole reason why we've been designing an entirely new thing. |
15:10 | <Andreu Botella> | I opened https://github.com/tc39/proposal-async-context/issues/90 to discuss the error event |
16:31 | <littledan> | By "mostly works" I mean it propagates correctly in most cases (callbacks), but async/await is not one of them. We have a pile of hacks to work around that currently lacking capability. |
16:32 | <littledan> | This wouldn't be the case, unless we somehow make the calling context be an array of the contexts of all promises that led to this point, which seems like clear overkill |
16:41 | <Chengzhong Wu> | So our main concern for APM uses is that code which is functionally equivalent from the user perspective should produce an equivalent context graph. In the example I posted yesterday (https://gist.github.com/Qard/6ceaca8bb792679e82c7693513baee0e) I have three examples of an http request using async/await, raw promises, and callbacks respectively. All three examples are functionally identical from user-facing execution flow perspective, yet they produce very different context graphs from AsyncContext because promises and async/await follow registration path rather than resolve path. |
17:11 | <Andreu Botella> | I hope it's not necessary either, but I can see how Ste{v,ph}en's points argue for it to be necessary. (I'm having trouble understanding what you're supposed to do with currentContext besides reconstruct this array) .run() to start a new span – but that would only work as long as you never await a promise that was created before the last await |
17:12 | <Andreu Botella> | so yeah, I also want to know what exactly Stephen had in mind there |
17:51 | <littledan> | I imagine that APMs could patch every built-in promise-returning API to get the span in the calling context, and then they could call the built-in inside |
22:17 | <Steve Hicks> | The "don't bind around await" case feels a lot like "just use a global variable" to me. To my mind, automatic binding around await is basically one of the main purposes of AC, so if you're not doing that, what are you gaining? Is it just the question of restoring context after awaiting an already-done promise? Assuming all awaited promises were pending when the await kicked off, then a simple model seems like doing nothing would "just work" since the continuation would follow immediately afterwards, so the global value would be correct. Of course this breaks down when the await doesn't actually have to wait - and maybe also when two independent jobs are running interleaved? So a global variable is obviously a bit too naive to work, but where I'm struggling is to understand how an AC.Variable with mutable state falls short? That seems like it would address the issue of interleaved independent tasks, at least, and since (within the same task) you're presumably not clobbering the actual container variable, then that part shouldn't need to change after an await (and if it did, it could presumably be communicated back upstream). ISTR Stephen mentioning performance concerns, but it's important to compare this against the performance cost of additional overhead needed to maintain a more complex graph, or more different kinds of state that need extra work to fork/join. |
22:23 | <Steve Hicks> | To Stephen's point about rewriting code from callbacks to promise/await, I agree that they should be consistent, but I'd argue that it's a transient issue. Callback-based code should be updated to wrap the callbacks so that the behavior is the same. I understand that the fact that ALS has existed for a while with the other behavior makes this a bit of a problem... Ideally the standard Node APIs would have all been updated to wrap/bind when ALS first came out, before anyone began depending on the non-wrapping behavior. |
22:26 | <Steve Hicks> | Now, the fact that this wrapping would make a handful of existing use cases impossible is clearly a problem, and points to why we need some way to access the causing context, rather than just the registration context. This could be as simple as a AsyncContext.Snapshot.noWrap that would prevent any automatic wrapping (or other implicit snapshot restoration that the VM would do via abstract operations), though it's not clear to me whether this would (or should!) provide the non-restoring async/await behavior (vs. just affecting what context callbacks are entered with the first time). |