The Wayback Machine - https://web.archive.org/web/20220322223447/https://github.com/nodejs/node/pull/33859
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

Jump to bottom

lib: remove manual exception handling in queueMicrotask #33859

Closed
wants to merge 2 commits into from
Closed

lib: remove manual exception handling in queueMicrotask #33859

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
devsnek added 2 commits Jun 12, 2020
@devsnek
deps: V8: cherry-pick 767e65f945e7
ce4d184 
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
@devsnek
lib: remove manual exception handling in queueMicrotask
b9f5307
@nodejs-github-bot nodejs-github-bot added the lib / src label Jun 12, 2020
addaleax
addaleax reviewed Jun 13, 2020
View changes
lib/internal/process/task_queues.js
} 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

@addaleax addaleax Jun 13, 2020

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

@devsnek devsnek Jun 13, 2020

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
Contributor

@nodejs-github-bot nodejs-github-bot commented Jun 13, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31857/
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jun 14, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31864/
@github-actions github-actions bot mentioned this pull request Jun 18, 2020
Open
18 tasks
@github-actions github-actions bot mentioned this pull request Jun 19, 2020
Open
36 tasks
@addaleax addaleax added dont-land-on-v10.x dont-land-on-v12.x labels Jun 19, 2020
addaleax added a commit that referenced this issue Jun 19, 2020
@devsnek @addaleax
deps: V8: cherry-pick 767e65f945e7
8f000ea 
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 added a commit that referenced this issue Jun 19, 2020
@devsnek @addaleax
lib: remove manual exception handling in queueMicrotask
178e52a 
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

@addaleax addaleax commented Jun 19, 2020

Landed in a86a295...178e52a
@addaleax addaleax closed this Jun 19, 2020
@devsnek devsnek deleted the proper-error-handling branch Jun 19, 2020
@codebytere codebytere added backport-requested-v14.x and removed backport-requested-v14.x labels Jun 27, 2020
codebytere added a commit that referenced this issue Jun 27, 2020
@devsnek @codebytere
deps: V8: cherry-pick 767e65f945e7
5957afc 
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 added a commit that referenced this issue Jun 27, 2020
@devsnek @codebytere
lib: remove manual exception handling in queueMicrotask
b6ef6c8 
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
Merged
codebytere added a commit that referenced this issue Jun 30, 2020
@devsnek @codebytere
deps: V8: cherry-pick 767e65f945e7
59ca593 
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 added a commit that referenced this issue Jun 30, 2020
@devsnek @codebytere
lib: remove manual exception handling in queueMicrotask
9a4a283 
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>
@Linkgoron Linkgoron mentioned this pull request Apr 12, 2021
Open
@Linkgoron Linkgoron mentioned this pull request Apr 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-land-on-v12.x lib / src
6 participants
@devsnek @nodejs-github-bot @addaleax @jasnell @devnexen @codebytere