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

lib: remove manual exception handling in queueMicrotask #33859

Closed
wants to merge 2 commits into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jun 12, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Original commit message:

    [API] Fix microtask message reporting

    RunSingleMicrotask calls Runtime::ReportMessage, but the implementation
    of ReportMessage would unconditionally discard these exceptions. This
    CL removes all of the intermediate logic and directly calls
    MessageHandler::ReportMessage, restoring the ability of
    RunSingleMicrotask to report exceptions that occur in microtasks.

    Bug: v8:8326
    Change-Id: I493de74383b2ab191d786611fb9eba9d27e7a243
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2162121
    Commit-Queue: Gus Caplan <me@gus.host>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67630}

Refs: v8/v8@767e65f
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 12, 2020
} catch (error) {
// runInAsyncScope() swallows the error so we need to catch
// it and handle it here.
triggerUncaughtException(error, false /* fromPromise */);
} finally {
this.emitDestroy();
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a check to verify that this emitDestroy() is still reached?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, test/parallel/test-queue-microtask-uncaught-asynchooks.js

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Jun 19, 2020
Original commit message:

    [API] Fix microtask message reporting

    RunSingleMicrotask calls Runtime::ReportMessage, but the implementation
    of ReportMessage would unconditionally discard these exceptions. This
    CL removes all of the intermediate logic and directly calls
    MessageHandler::ReportMessage, restoring the ability of
    RunSingleMicrotask to report exceptions that occur in microtasks.

    Bug: v8:8326
    Change-Id: I493de74383b2ab191d786611fb9eba9d27e7a243
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2162121
    Commit-Queue: Gus Caplan <me@gus.host>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67630}

Refs: v8/v8@767e65f

PR-URL: #33859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 19, 2020
PR-URL: #33859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member

Landed in a86a295...178e52a

@addaleax addaleax closed this Jun 19, 2020
@devsnek devsnek deleted the proper-error-handling branch June 19, 2020 17:04
codebytere pushed a commit that referenced this pull request Jun 27, 2020
Original commit message:

    [API] Fix microtask message reporting

    RunSingleMicrotask calls Runtime::ReportMessage, but the implementation
    of ReportMessage would unconditionally discard these exceptions. This
    CL removes all of the intermediate logic and directly calls
    MessageHandler::ReportMessage, restoring the ability of
    RunSingleMicrotask to report exceptions that occur in microtasks.

    Bug: v8:8326
    Change-Id: I493de74383b2ab191d786611fb9eba9d27e7a243
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2162121
    Commit-Queue: Gus Caplan <me@gus.host>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67630}

Refs: v8/v8@767e65f

PR-URL: #33859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 27, 2020
PR-URL: #33859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
Original commit message:

    [API] Fix microtask message reporting

    RunSingleMicrotask calls Runtime::ReportMessage, but the implementation
    of ReportMessage would unconditionally discard these exceptions. This
    CL removes all of the intermediate logic and directly calls
    MessageHandler::ReportMessage, restoring the ability of
    RunSingleMicrotask to report exceptions that occur in microtasks.

    Bug: v8:8326
    Change-Id: I493de74383b2ab191d786611fb9eba9d27e7a243
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2162121
    Commit-Queue: Gus Caplan <me@gus.host>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67630}

Refs: v8/v8@767e65f

PR-URL: #33859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
PR-URL: #33859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants