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

Standardise increasing the event limit for events #105

Open
tegefaulkes opened this issue Jun 5, 2024 · 4 comments
Open

Standardise increasing the event limit for events #105

tegefaulkes opened this issue Jun 5, 2024 · 4 comments
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices

Comments

@tegefaulkes
Copy link
Contributor

Specification

Right now in certain cases we have an EventTarget or a AbortSignal which has many registered listeners. Usually when multiple children classes need to listen to a parent. EG, multiple QUICConnections per QUICServer, or multiple QUICStreams per QUICConnection. By default, when we exceed 11 listeners we get the following warning.

> (node:61422) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 foo listeners added to [EventEmitter]. Use emitter.setMaxListeners() to increase limit

Currently this has been addressed in two locations with the following.

QUICServer.ts 379: nodesEvents.setMaxListeners(100000, this.stopAbortController.signal); QUICSocket.ts 254: nodesEvents.setMaxListeners(100000, this[_eventTarget]);

But we want to standardise how this is generally done and avoid magic numbers in random places in the code. The limit should also configurable in some way. At the very least the nodesEvents.setMaxListeners should be moved into a utility function to avoid spreading out the magic.

Additional context

Relevant discussion: https://github.com/MatrixAI/js-quic/pull/104/files#r1626356768

Tasks

  1. The nodesEvents.setMaxListeners usage should be standardised and quarantined into a utility function.
  2. The limit should come from a config parameter in some way.
@tegefaulkes tegefaulkes added the development Standard development label Jun 5, 2024
Copy link

linear bot commented Jun 5, 2024

@tegefaulkes
Copy link
Contributor Author

This was addressed by the recent commit 0d7bc46

@CMCDragonkai
Copy link
Member

This:

/**
 * Increases the total number of registered event handlers before a node warning is emitted.
 * In most cases this is not needed but in the case where you have one event emitter for multiple handlers you'll need
 * to increase the limit.
 * @param target - The specific `EventTarget` or `EventEmitter` to increase the warning for.
 * @param limit - The limit before the warning is emitted, defaults to 100000.
 */
function setMaxListeners(
  target: EventTarget | NodeJS.EventEmitter,
  limit: number = 100000,
) {
  events.setMaxListeners(limit, target);
}

The number there needs to be part of the system configuration config.ts whether in js-quic or in Polykey... etc. It's a magic number and magic numbers must be lifted to be explicit.

@tegefaulkes
Copy link
Contributor Author

Ok, I'll make the change.

@tegefaulkes tegefaulkes reopened this Jul 21, 2024
@tegefaulkes tegefaulkes removed their assignment Jul 31, 2024
@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices
Development

No branches or pull requests

2 participants