Skip to content

fix: improve event-loop promise state tracking#52108

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Uzlopak:improve-event-loop
May 6, 2024
Merged

fix: improve event-loop promise state tracking#52108
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Uzlopak:improve-event-loop

Conversation

@Uzlopak

@Uzlopak Uzlopak commented Mar 15, 2024

Copy link
Copy Markdown
Contributor

Fixes #43655
Fixes #34851
Superseeds #34862

@nodejs/performance

Can you help me with the changes in my last commit? I adapted changes from #34862. This should improve the performance of rejected promises. Maybe you have some concerns regarding the the changes. I am not that convinced from the last commit.

Ty

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Mar 15, 2024
@Uzlopak Uzlopak changed the title chore: improve event-loop Mar 15, 2024
@Uzlopak Uzlopak force-pushed the improve-event-loop branch 3 times, most recently from b4b0a4c to d221af4 Compare March 16, 2024 03:02
Comment thread lib/internal/process/promises.js Outdated
@Uzlopak Uzlopak changed the title chore: improve event-loop promise state tracking Mar 17, 2024
@Uzlopak Uzlopak requested a review from aduh95 March 17, 2024 17:07

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024
@mcollina

mcollina commented Mar 18, 2024

Copy link
Copy Markdown
Member

I think we should remove multipleResolves event in a follow-up PR.

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024
@Uzlopak

Uzlopak commented Mar 20, 2024

Copy link
Copy Markdown
Contributor Author

@mcollina
@anonrig

Is there something i have to do?

@aduh95

aduh95 commented Mar 20, 2024

Copy link
Copy Markdown
Contributor

Yes, the added test is flaky:

not ok 2290 parallel/test-promise-unhandled-issue-43655
  ---
  duration_ms: 1031.49800
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/process/promises:417
        triggerUncaughtException(err, true /* fromPromise */);
        ^
    
    AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
    
      assert.ok(Date.now() - time0 < 10)
    
        at test (/Users/iojs/build/workspace/node-test-commit-osx-arm/nodes/osx11/test/parallel/test-promise-unhandled-issue-43655.js:23:10) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: false,
      expected: true,
      operator: '=='
    }
    
    Node.js v22.0.0-pre
  ...
@Uzlopak

This comment was marked as outdated.

@Uzlopak Uzlopak force-pushed the improve-event-loop branch from 13e0e13 to 014d1b5 Compare March 21, 2024 02:13
@Uzlopak

Uzlopak commented Mar 21, 2024

Copy link
Copy Markdown
Contributor Author

It should be now less flaky.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2024

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2024

@mertcanaltin mertcanaltin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread lib/internal/process/promises.js
Comment thread test/fixtures/errors/unhandled_promise_trace_warnings.snapshot
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2024
@Uzlopak

Uzlopak commented Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

Are the test failures related to my patch? I have no osx-arm machine to check it.

@Uzlopak Uzlopak force-pushed the improve-event-loop branch 2 times, most recently from 71b3502 to 691b724 Compare April 24, 2024 12:27
@BridgeAR

Copy link
Copy Markdown
Member

@Uzlopak they do not seem to be related

Comment thread lib/internal/process/promises.js Outdated
@Uzlopak Uzlopak force-pushed the improve-event-loop branch from 691b724 to a5dc9a3 Compare May 6, 2024 03:22
@Uzlopak

Uzlopak commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

@BridgeAR
PTAL

@Uzlopak Uzlopak force-pushed the improve-event-loop branch from a5dc9a3 to 6fd930b Compare May 6, 2024 03:52

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still lgtm

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2024
@Uzlopak

Uzlopak commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

Whoa!!!! the CI is green. the CI is green!

@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2024
@nodejs-github-bot nodejs-github-bot merged commit be8d64e into nodejs:main May 6, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in be8d64e

@Uzlopak Uzlopak deleted the improve-event-loop branch May 6, 2024 20:18
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this pull request May 8, 2024
PR-URL: nodejs#52108
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request May 8, 2024
PR-URL: #52108
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
PR-URL: #52108
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
PR-URL: nodejs#52108
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
PR-URL: nodejs#52108
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.

10 participants