fix: unset context on stale promises (slightly different approach) #16936
Conversation
|
|
| d.resolve(v); | ||
| } | ||
| }, d.reject); | ||
| Promise.resolve(fn()).then(d.resolve, d.reject).then(unset_context); |
There was a problem hiding this comment.
It would be good to have a comment explaining why we want to have it up here in addition to the handler below, but otherwise lgtm 👍
There was a problem hiding this comment.
actually you know what? we don't need the unset_context below, just here (and in the catch clause)
There was a problem hiding this comment.
I don't think so - when you reject a stale promise you'll end up in the handler and need to unset the context there because it doesn't run this line. The previous version was more correct
There was a problem hiding this comment.
Why? The point of #16935 is that context is saved in fn() but may not be correctly unset if the promise is forcibly rejected before fn() resolves. When that happens, there's nothing to unset, surely?
There was a problem hiding this comment.
Most likely yes, if everything's correct elsewhere it should be null going in. So yeah, I guess your change is fine
* fix: unset context on stale promises When a stale promise is rejected in `async_derived`, and the promise eventually resolves, `d.resolve` will be noop and `d.promise.then(handler, ...)` will never run. That in turns means any restored context (via `(await save(..))()`) will never be unset. We have to handle this case and unset the context to prevent errors such as false-positive state mutation errors * fix: unset context on stale promises (slightly different approach) (#16936) * slightly different approach to #16935 * move unset_context call * get rid of logs --------- Co-authored-by: Rich Harris <rich.harris@vercel.com>
2 participants
slightly different approach to #16935