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

fix: let Node.js perform microtask checkpoint in the main process #24131

Merged
merged 7 commits into from Jun 17, 2020

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Jun 15, 2020

Description of Change

Closes #23838.
Refs #20013 (comment)

Checklist

Release Notes

Notes: fix delayed execution of some Node.js callbacks in the main process

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 15, 2020
@codebytere
Copy link
Member

codebytere commented Jun 15, 2020

@deepak1556 i think this fixes #23838

const f3 = async () => {
    return new Promise((_, reject) => {
        reject('oops');
    });
}

f3().catch((error) => {
   console.log('3 caught', error);
});

should function as a minimal test case if run in the main proc

Right now it errors like so:

(node:86234) UnhandledPromiseRejectionWarning: oops
(node:86234) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:86234) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
3 caught oops
(node:86234) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)

because the catch is happening on a subsequent tick, one farther than it should, and there are two promises happening (the async wrapper and the promise itself) which lines up with what you're describing here

See #23838 (comment) :)

@deepak1556
Copy link
Member Author

Thanks @codebytere , yep that does serve as a valid test case.

@deepak1556 deepak1556 force-pushed the robo/fix_microtask_checkpoint branch from 9836376 to 7d48eb0 Compare June 16, 2020 01:03
@deepak1556 deepak1556 force-pushed the robo/fix_microtask_checkpoint branch from 7d48eb0 to 9b6ef9a Compare June 16, 2020 01:22
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 16, 2020
We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.
We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.
@zcbenz
Copy link
Member

zcbenz commented Jun 16, 2020

On the failed getGPUInfo test, it is because with this change the resolve/reject callbacks get called earlier and then results in a call like this:

w.webContents.once('did-finish-load', () => {
  app.exit(1)
})

Which then crashes.

Schedule the app.exit into next tick in gpu-info.js should be able to fix the failed test, though we should look into solving the crash in future.

@deepak1556 deepak1556 merged commit 7cc780d into master Jun 17, 2020
@release-clerk
Copy link

release-clerk bot commented Jun 17, 2020

Release Notes Persisted

fix delayed execution of some Node.js callbacks in the main process

@trop
Copy link
Contributor

trop bot commented Jun 17, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/8-x-y label Jun 17, 2020
@trop
Copy link
Contributor

trop bot commented Jun 17, 2020

I was unable to backport this PR to "9-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jun 17, 2020

I have automatically backported this PR to "10-x-y", please check out #24174

@deepak1556 deepak1556 deleted the robo/fix_microtask_checkpoint branch June 17, 2020 17:08
deepak1556 added a commit that referenced this pull request Jun 17, 2020
…4131)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec
@trop
Copy link
Contributor

trop bot commented Jun 17, 2020

@deepak1556 has manually backported this PR to "9-x-y", please check out #24178

deepak1556 added a commit that referenced this pull request Jun 17, 2020
…4131)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec
@trop
Copy link
Contributor

trop bot commented Jun 17, 2020

@deepak1556 has manually backported this PR to "8-x-y", please check out #24180

deepak1556 added a commit that referenced this pull request Jun 17, 2020
…4131)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec
deepak1556 added a commit that referenced this pull request Jun 17, 2020
…4131) (#24180)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec
deepak1556 added a commit that referenced this pull request Jun 17, 2020
…4131) (#24178)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec
@trop trop bot added the merged/9-x-y label Jun 17, 2020
sentialx pushed a commit to sentialx/electron that referenced this pull request Jul 30, 2020
…ectron#24131)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec
sentialx pushed a commit to sentialx/electron that referenced this pull request Apr 8, 2021
…ectron#24131)

* fix: let Node.js perform microtask checkpoint in the main process

* fix: don't specify v8::MicrotasksScope for explicit policy

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix: remove checkpoint from some call-sites

We already perform checkpoint at the end of a task,
either through MicrotaskRunner or through NodeBindings.
There isn't a need to add them again when calling into JS
except when dealing with promises.

* fix incorrect specs

* default constructor arguments are considered for explicit mark

* add regression spec
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.

Incorrect UnhandledPromiseRejectionWarning for asynchronous called async function
3 participants