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

fix: set target socket max listeners to infinity #570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jirimoravcik
Copy link
Member

@jirimoravcik jirimoravcik commented Jan 22, 2025

It happens that targetSocket has many .on('close', ...) listeners and we get MaxListenersExceededWarning. I increase the limit to infinity so we don't see such warnings.

I'd guess this is due to the fact that some target sockets can be reused/kept alive and therefore they have more on close listeners attached from several source sockets.

@github-actions github-actions bot added this to the 107th sprint - Platform team milestone Jan 22, 2025
@github-actions github-actions bot added the t-platform Issues with this label are in the ownership of the platform team. label Jan 22, 2025

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

@@ -88,6 +88,9 @@ export const chain = (
client.on('connect', (response, targetSocket, clientHead) => {
countTargetBytes(sourceSocket, targetSocket);

// There may be many .on('close') listeners, so we need to increase the limit.
targetSocket.setMaxListeners(Infinity);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this risky? The warning is there to help detect memory leaks, now we turn this off and if proxy starts crashing, we won't even know why. Maybe increase it too like 1000 instead?

Copy link
Member

@fnesveda fnesveda left a comment

Choose a reason for hiding this comment

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

How about attaching the handlers with with .once('close') instead of .on('close')? That might actually solve the issue.

@jirimoravcik
Copy link
Member Author

How about attaching the handlers with with .once('close') instead of .on('close')? That might actually solve the issue.

Based on what would that solve the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-platform Issues with this label are in the ownership of the platform team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants