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

unhandled promise rejection mem leak #3478

Closed
mattkrick opened this issue Jan 12, 2020 · 2 comments
Closed

unhandled promise rejection mem leak #3478

mattkrick opened this issue Jan 12, 2020 · 2 comments

Comments

@mattkrick
Copy link
Member

  • We've got a big memory leak that is top priority
  • There are 3 moving parts: node, uWebSockets, rethinkdb-ts
  • rethinkdb-ts does not properly handle a certain promise "No more rows in the cursor."
  • that error includes the dataloader in the StackTraceFrame, which is what makes the error so large
  • since it's not properly handled, node pushes it to an array: https://github.com/nodejs/node/blob/c9b93e234454322ac0b7a6cd29d394f428f3e37d/lib/internal/process/promises.js#L102
  • for some reason, that array is not getting flushed
  • to flush the array, processPromiseRejections must get called
  • calling that requires processTicksAndRejections get called
  • that above should get called every tick per setTickCallback, which is C++ code
  • since this was working before, i assume uWebSockets is monkeying around with node so no tick is called
@mattkrick
Copy link
Member Author

so, next steps are:

  • see if the bug goes away with node 13 (latest)
  • patch rethinkdb-ts
  • hope a fix comes soon for uWebSockets, or we'll likely move backwards to a vanilla node + cws, not ideal.

@mattkrick
Copy link
Member Author

  • node v13.6.0 fixed the bug so RethinkDBError is no longer treated as an unhandled rejection
  • new node version may have a side effect that doesn't play well with datadog apm. welcome to javascript 😒
  • uWebSockets will likely fallback to MakeCallback, which will result in a performance hit, but will fix this bug & improve stability
  • looks like we've still got a small mem leak (maybe 1MB/hr?). not enough to warrant spending any more time on this, but i may do another dump this weekend just to see what it is

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

No branches or pull requests

1 participant