Align process.nextTick execution order with Node - #4409
Conversation
|
❌ @paperdave 7 files with test failures on bun-darwin-aarch64:
|
|
❌ @paperdave 2 files with test failures on linux-x64:
|
|
❌ @paperdave 2 files with test failures on linux-x64-baseline:
|
|
❌ @paperdave 8 files with test failures on bun-darwin-x64-baseline:
|
There was a problem hiding this comment.
I've been checking with node and this implementation is incorrect, nextTick runs after queueMicrotask as of node 20. Hopefully switching this around isn't too hard, but I don't think onEachMicrotaskTick will do the trick.
let resolve;
let promise = new Promise(res => {
resolve = res;
});
const order = [];
let nextTickI = 0;
let microtaskI = 0;
let remaining = 20;
let runs = [];
for (let i = 0; i < 10; i++) {
queueMicrotask(() => {
runs.push(queueMicrotask);
order.push("queueMicrotask " + microtaskI++);
if (--remaining === 0) resolve(order);
});
process.nextTick(() => {
runs.push(process.nextTick);
order.push("process.nextTick " + nextTickI++);
if (--remaining === 0) resolve(order);
});
}
await promise;
console.log(order);yields:
I'm going to update the tests to reflect this, as well as use jest as a baseline to ensure that they pass on node too.
There was a problem hiding this comment.
why is the handler cleared here? it doesn't seem to ever be restored.
also we now need to coordinate this with the async_hooks setOnEachMicrotaskTick overwriting this handler temporarily.
I will push some tests that hit these edge cases
|
Actually, I'm incorrect. Having this not be at the top level in node causes the order to be as you implemented it. going off my prev script, let resolve;
let promise = new Promise(res => {
resolve = res;
});
const order = [];
let nextTickI = 0;
let microtaskI = 0;
let remaining = 20;
let runs = [];
for (let i = 0; i < 10; i++) {
+ process.nextTick(() => {
queueMicrotask(() => {
runs.push(queueMicrotask);
order.push("queueMicrotask " + microtaskI++);
if (--remaining === 0) resolve(order);
});
process.nextTick(() => {
runs.push(process.nextTick);
order.push("process.nextTick " + nextTickI++);
if (--remaining === 0) resolve(order);
});
+ });
}
await promise;
console.log(order);
I'm sure you checked this page: https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick#phases-overview |
|
Tests do not pass, we need to address the exit code issue and possibly this readline promises test failure |
abc7862 to
b55d5aa
Compare
|
Let’s merge after tests run (you’ll need to fix the conflicts) |
b55d5aa to
78f9a0c
Compare
2 participants
What does this PR do?
In Node,
process.nextTickruns before microtasks are drained. Previously, Bun was treatingprocess.nextTickas the same queue as the microtask queue. This led to subtle bugs.Fixes #4252, probably others
The code is loosely based on https://github.com/nodejs/node/blob/main/lib/internal/process/task_queues.js. The main difference is that our implementation is lazier and we don't emit on unhandledexception callbacks currently.
How did you verify your code works?
A couple tests. Have to avoid referencing the process object inside outer function to avoid causing it to infinite loop. Using the
$createFIFObuiltin creates too large of an array in stress tests, causing an infinite loop. That's why it uses this fixed buffer abstraction from node.I think we will need to make one small tweak to support AsyncLocalStorage, but @paperdave will know more about that than me