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

Support Promise execution in the sandbox. #872

Merged
merged 2 commits into from Nov 18, 2022

Conversation

kevinswiber
Copy link
Member

@kevinswiber kevinswiber commented Nov 7, 2022

Promise.prototype.then arguments are executed in a microtask queue,
pushed to the end of the current event loop tick. The async
lifetime tracking in the sandbox did not factor this into
consideration. Certain completion functions needed to be deferred
with setImmediate(), which will add to the task queue and
execute after the current microtask queue drains.

Calls to global timers also needed to be wrapped in a scoped context.
Due to the setImmediate() delay, the bridge may disconnect when the
context is disposed before the completion timer fires. Disconnecting
the bridge removes all globals. This prevents reference errors from
happening on the final tick.

Original behavior of execution.return.async remains. It will only
be set to true if there's a pending timer that hasn't been
previously cleared.

A code example that will show the current issue of early exit:

const foo = () => new Promise((resolve) => {
  console.log('foo');
  setTimeout(resolve, 200);
});

const bar = () => new Promise((resolve) => {
  console.log('bar');
  resolve();
});

const baz = () => new Promise((resolve) => {
  console.log('baz');
  resolve();
});

Promise.resolve()
  .then(() => foo())
  .then(() => bar())
  .then(() => baz());

The issue occurs when async tracking happens within the microtask queue. Without these changes, the sandbox believes all async functions are done and exits. Using setImmediate in a couple of places defers that check to the beginning of the next tick, after the current tick's microtask queue is drained.

@kevinswiber
Copy link
Member Author

Note that this PR does not address unhandled Promise rejections. That will need to be a separate PR to the https://github.com/postmanlabs/uvm repo.

@kevinswiber kevinswiber force-pushed the promise-support branch 2 times, most recently from f2640b1 to dba39f9 Compare November 9, 2022 17:37
Promise.prototype.then arguments are executed in a microtask queue,
pushed to the end of the current event loop tick. The async
lifetime tracking in the sandbox did not factor this into
consideration. Certain completion functions needed to be deferred
with setImmediate(), which will add to the task queue and
execute after the current microtask queue drains.

Calls to global timers also needed to be wrapped in a scoped context.
Due to the setImmediate() delay, the bridge may disconnect when the
context is disposed before the completion timer fires. Disconnecting
the bridge removes all globals. This prevents reference errors from
happening on the final tick.

Original behavior of execution.return.async remains.  It will only
be set to `true` if there's a pending timer that hasn't been
previously cleared.
!clearing && (typeof onAllTimerEnd === FUNCTION) && onAllTimerEnd();
computeTimerEvents.started = false;
function maybeEndAllTimers () {
if (pending === 0 && computeTimerEvents.started) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a duplicate check from line 167.

Copy link
Member Author

Choose a reason for hiding this comment

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

@codenirvana The first check is "only queue this if pending is currently at 0." The second check is "only actually run this if pending is still at 0." This is the one we really need. The first check just prevents needless queueing.

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #872 (f019c7a) into develop (8fae465) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #872   +/-   ##
========================================
  Coverage    60.71%   60.71%           
========================================
  Files           12       12           
  Lines          560      560           
  Branches       135      135           
========================================
  Hits           340      340           
  Misses         220      220           
Flag Coverage Δ
unit 60.71% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/sandbox/timers.js 74.50% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@codenirvana codenirvana merged commit 8209483 into postmanlabs:develop Nov 18, 2022
codenirvana added a commit that referenced this pull request Apr 5, 2024
Support Promise execution in the sandbox.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants