Hi Leszek, On Fri, Sep 5, 2025 at 7:36 AM Leszek Swirski <[email protected]> wrote:
> It sounds like you've found a bug in our C++ implementation which is > incompatible with the spec; I'd have to read into the exact details but it > sounds plausible. This is great, and we should fix this, however your > subsequent comments sound like they want the broken behaviour? > Something in between. While the C++ code is closer, I think we'd want a variant: we want the C++ behavior only if the resolution is an already-fulfilled promise, otherwise we want the Torque behavior. > In general, for us, spec is non-negotiable (the few times we do break spec > is for web compatibility or some serious performance concern), so if you > think a break from spec behaviour would be useful for Node, then you'd want > to make that a spec proposal. > Understood. The nested microtask queues Node is using sound a bit broken, since it > sounds like the microtask queue is only drained once? If I understand the > issue correctly, then the right fix would be for Node to always drain the > inner microtask queue after something executes in the inner realm, or, I > guess, patch the native context after draining it to refer to the outer > microtask queue. > I did consider patching the context to point to the outer queue after evaluate(), or just set the queue to null, but in typical node:vm usage this context object may be shared with other (inner) modules that may be evaluated later, so I don't think we can actually modify it. Every time some code awaits on the already-fulfilled promise returned by evaluate(), a new (trivial) task will appear on the inner microtask queue, otherwise (long) inactive. The issue for Node is that there is no way of knowing that the inner microtask queue should be drained. This (trivial) task gets enqueued after inner_module.evaluate() returns, at a point where we are done with inner_module, and I'm not sure where there would be a reasonable place to check "what if the queue for this other context we're not using anymore needs draining?" Another way to describe the issue is that with two contexts A and B, with queues QA and QB, context B is effectively waiting on something to happen in context A (the trivial thenable job task), but while there is a task enqueued on QA, there are no tasks enqueued on QB to indicate that the (outer) module B has more work to do, leading to the evaluation of module B to just terminate early. The lack of dependency tracking between contexts B and A is what is broken in the spec, I think. Whenever context B enqueues something onto QA, it is possible (but not guaranteed) that A will in turn enqueue something back onto QB. So, context B should be aware of that possibility and consider itself waiting on a QA checkpoint. It's only after a QA checkpoint is performed that context B can check if its queue is effectively empty or not. Possibly, In Node or in v8::SourceTextModule, after finding that the current queue is empty, before terminating the evaluation of the current module, we could check if any other "related" microtask queues are non-empty and drain them, before checking if doing this added any tasks to our current queue. Or, after all, it may be worth considering another option I included in comment #3: "In V8, add a callback to the public API MicrotaskQueue to be notified, with policy kExplicit: either when a task is enqueued, or more efficiently, when there are enqueued tasks in strategic places (maybe in the same places where, with policy kAuto, a checkpoint may be performed). This would let Node know that the qeueue associated with an already-evaluated module needs to be drained again. I have not tried this, but it seems very clunky." This callback could be associated with the queue: OnMicrotaskEnqueue(microtask_queue). In Node, we would maintain enough state to know if we've entered a context associated with this queue. In the callback: if the queue is in an entered context, we know that the queue will get drained later, we have nothing to do; but if this queue is not associated with an entered context, we immediately drain the queue. Or this callback could be associated with the native context: OnMicrotaskEnqueue(native_context). The Node implementation of this callback would be simpler, as we only need to check if native_context matches the isolate current native context. Thanks, Eric -- -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion visit https://groups.google.com/d/msgid/v8-dev/CA%2BzRj8VUzFOiqhCKEzKpNbU-xN6KnKCNtTai%3DdMv5zXA%2BYzdEw%40mail.gmail.com.
