Skip to content

Fix memory leak in Http2SessionManager's verify step#1616

Merged
timostamm merged 12 commits intoconnectrpc:mainfrom
ttyniwa:feature/fix-memory-leak-in-http2sessionmanager
Nov 17, 2025
Merged

Fix memory leak in Http2SessionManager's verify step#1616
timostamm merged 12 commits intoconnectrpc:mainfrom
ttyniwa:feature/fix-memory-leak-in-http2sessionmanager

Conversation

@ttyniwa
Copy link
Contributor

@ttyniwa ttyniwa commented Nov 11, 2025

Background

Within the verify function of Http2SessionManager, error event listeners were not being unregistered upon success, resulting in a leak.
The following error log was observed:
(node:1)MaxListenersExceededWarning: Possible Event Emitter memory leak detected. 11 error listeners added to [ClientHttp2Session]. MaxListeners is 10. Use emitter.setMaxListeners() to increase limit

Summary

Fixed a bug causing MaxListenersExceededWarning in Http2SessionManager.

Signed-off-by: ttyniwa <noencrypt@gmail.com>
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I can reproduce the issue, see #1617.

We can simplify the code a bit, see comments below. Happy to merge this fix with those changes.

@ttyniwa ttyniwa requested a review from timostamm November 14, 2025 14:46
@ttyniwa
Copy link
Contributor Author

ttyniwa commented Nov 14, 2025

@timostamm Thank you for your review.
I fixed all.

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Nice. Let's get this merged. But there are two issue to fix before we can merge this:

  1. The code isn't formatted correctly. You can run npm run format --workspace packages/connect-node to fix that.
  2. You'll have to sign your commits. See the DCO check for more information on how to sign.

dependabot bot and others added 10 commits November 15, 2025 15:14
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timo Stamm <ts@timostamm.de>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timo Stamm <ts@timostamm.de>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Timo Stamm <ts@timostamm.de>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timo Stamm <ts@timostamm.de>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Timo Stamm <ts@timostamm.de>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Timo Stamm <ts@timostamm.de>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
Signed-off-by: ttyniwa <noencrypt@gmail.com>
@ttyniwa ttyniwa force-pushed the feature/fix-memory-leak-in-http2sessionmanager branch from 471b0d4 to 7a10a4c Compare November 15, 2025 06:16
@ttyniwa ttyniwa requested a review from timostamm November 15, 2025 06:39
@ttyniwa
Copy link
Contributor Author

ttyniwa commented Nov 15, 2025

Thanks for your support. Fixed it.

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

This fixes the issue from the reproduction in #1617

@timostamm timostamm changed the title Fix memory leak in Http2SessionManager Fix memory leak in Http2SessionManager's verify step Nov 17, 2025
@timostamm timostamm merged commit 280adb2 into connectrpc:main Nov 17, 2025
31 checks passed
@timostamm timostamm mentioned this pull request Nov 17, 2025
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

Successfully merging this pull request may close these issues.

2 participants