Skip to content
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

test(ext/ffi): Increase timeout value in event loop integration test callback #18394

Merged

Conversation

aapoalas
Copy link
Collaborator

The test has flakes for essentially the same reason as what it is trying to test: How asynchronous work started from FFI callbacks gets resolved.

Due to Deno using the v8::MicrotasksPolicy::kAuto for handling microtasks, the microtask queue is drained synchronously when the script call depth decrements to zero. FFI callbacks coming from foreign threads get called one at a time from an empty scope created expressly for the purpose of calling that particular callback. As a result, each FFI callback immediately drains the microtask queue upon its completion.

The test uses two calls, queueMicrotask and setTimeout to respectively push one microtask into the queue and push one macrotask into their separate queue. However, setTimeout isn't exactly guaranteed to create a macrotask. A short enough timeout can cause the task to become a microtask instead. The 10 ms timeout had been chosen as a safe bound but apparently it is not quite safe enough. The flake occurs when the setTimeout manages to inch itself into the microtask queue:

The test expects that first the "Sync" will be logged from the callback directly, then "Async" from the queueMicrotask microtask. Then, the Rust code calling the callback will log its "STORED_FUNCTION called", after which finally the setTimeout task logs "Timeout". When setTimeout gets into the microtask queue, it instead logs before the Rust code is returned to.

From a JS point of view this would look like this to:

(() => {
  console.log("Sync");
  queueMicrotask(() => console.log("Async"));
  setTimeout(() => console.log("Timeout"), 10);
})();
console.log("Done");

@aapoalas aapoalas linked an issue Mar 23, 2023 that may be closed by this pull request
@aapoalas aapoalas requested review from crowlKats and dsherret and removed request for crowlKats March 23, 2023 18:26
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @aapoalas!

@aapoalas aapoalas merged commit 1c6b797 into denoland:main Mar 25, 2023
@aapoalas aapoalas deleted the test/ext-ffi/event-loop-integration-is-flaky branch March 25, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

event_loop_integration is flaky
2 participants