Skip to content

Problem with GC (million promise rejections) #43655

Closed
#52108
Closed
Problem with GC (million promise rejections)#43655
#52108
Labels
confirmed-bugIssues and PRs for confirmed bugs.v8 engineIssues and PRs related to the V8 dependency.

Description

@NikolayMakhonin
NikolayMakhonin
opened on Jul 2, 2022

Version

16.15.1

Platform

Microsoft Windows NT 10.0.19042.0 x64

Subsystem

No response

What steps will reproduce the bug?

Just run it in the Node.JS <= 18:

(I have tested it on node v14.19.3, v16.15.1, v18.2.0)

test.js

function delay(time) {
  return new Promise((resolve) => {
    setTimeout(resolve, time)
  })
}

async function test() {
  for (let i = 0; i < 1000000; i++) {
    await new Promise((resolve, reject) => {
      reject('value')
    })
      .then(() => {}, () => {})

    // if (i % 10000 === 0) {
    //  await delay(100)
    // }
  }

  console.log('OK')

  const time0 = Date.now()
  await delay(0)
  console.log('Real delay time: ' + (Date.now() - time0))
}

test()

The test function will generate 1 million of Promise rejections. And after finish it will hang. It seems like the garbage collection called on idle, and it takes very long time.
If you replace the reject with the resolve, then everything will work quickly.
You can also uncomment the periodically delay, and it will work quickly.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

Work without hungs

Additional information

I can reproduce it in the Chromium 49 but it work correctly in the Chrome latest. It looks like this bug has been fixed in the Chrome.

This bug is easy to work around, but now I have the same bug with 30000 call of worker functions. I can't describe it yet, because I need to research it before. I think maybe the promise rejection bug will expose a wider problem and I won't have to create a new issue.

Activity

  1. bnoordhuis
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    v8 engineIssues and PRs related to the V8 dependency.
    on Jul 3, 2022

  2. bnoordhuis commented on Jul 3, 2022

    @bnoordhuis
    bnoordhuis
    on Jul 3, 2022
    Member

    I can confirm the issue and I speculate it's caused by how V8 internally tracks rejected promises.

    Here is a reduced test case:

    // Flags: --predictable
(async () => {
  for (let i = 0; i < 36_293; i++) // fast - 36_294 is 3x slower
    await new Promise((_, g) => g()).then(() => {}, () => {}) // not .catch
})()

    What's remarkable is that performance is not linear. A lower value like 30_000 is 2x slower than 36_923.

  3. Slayer95 commented on Jul 3, 2022

    @Slayer95
    Slayer95
    on Jul 3, 2022
    Contributor

    I see a few similarities with #29385. Maybe they are related?

  4. benjamingr commented on Jul 28, 2022

    @benjamingr
    benjamingr
    on Jul 28, 2022
    Member

    Rejections are significantly more costly than fulfillment (e.g. because of rejection tracking) which is "by design".

    Exceptional control flow prioritizes reliability and debuggability over performance and whenever we discussed what to make faster in promises the consensus was always that error flow performance isn't important since errors are rare.

  5. benjamingr commented on Jul 28, 2022

    @benjamingr
    benjamingr
    on Jul 28, 2022
    Member

    @bnoordhuis

    A lower value like 30_000 is 2x slower than 36_923.

    I suspect that since we push to a WeakMap it does something differently when it reaches a threshold.

    Comment out setPromiseRejectCallback and see if that "resolves" the performance difference.

    Oh and @NikolayMakhonin :

    I can reproduce it in the Chromium 49 but it work correctly in the Chrome latest.

    The promise impl was entirely swapped and replaced/improved in Chrome since Chrome 49 :)

  6. benjamingr commented on Jul 28, 2022

    @benjamingr
    benjamingr
    on Jul 28, 2022
    Member

    I'll open a PR to not save promises in the map if the unhandled rejection mode is "none". Note the cost of not saving rejections is that you will not get notified of unhandled rejections if that helps

  7. targos commented on Nov 8, 2022

    @targos
    targos
    on Nov 8, 2022
    Member

    @benjamingr Do you still want to open that PR?

  8. benjamingr commented on Nov 11, 2022

    @benjamingr
    benjamingr
    on Nov 11, 2022
    Member

    @NikolayMakhonin can you confirm that the fix (working with --unhandled-rejections=none) would address the issue from your point of view?

  9. itzmanish commented on Mar 19, 2023

    @itzmanish
    itzmanish
    on Mar 19, 2023

    I think this may be related to issue #47158 which I recently created.

  10. NikolayMakhonin commented on Apr 29, 2023

    @NikolayMakhonin
    NikolayMakhonin
    on Apr 29, 2023
    Author

    I tested this again on Node 16.20.0, the issue persists.

    I also tried --unhandled-rejections=none, as @benjamingr suggested above, but it didn't help.

    Now I'm getting this error not in the tests, but in a real application that synchronize millions of files between hard drives. There is the same thing: Node hangs, CPU is fully loaded, but JavaScript is doing nothing.

  11. bnoordhuis commented on Apr 29, 2023

    @bnoordhuis
    bnoordhuis
    on Apr 29, 2023
    Member

    I also tried --unhandled-rejections=none, as @benjamingr suggested above, but it didn't help.

    I don't think that change was ever merged (or PR'd.)

  12. nodejs-github-bot
    closed this as completedin #52108on May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.v8 engineIssues and PRs related to the V8 dependency.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions