MatrixAI / js-quic

QUIC Networking for TypeScript & JavaScript
https://matrixai.github.io/js-quic/
Apache License 2.0
13 stars 1 forks source link

Standardise increasing the event limit for events #105

Open tegefaulkes opened 3 months ago

tegefaulkes commented 3 months ago

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.
linear[bot] commented 3 months ago

ENG-330 Standardise increasing the event limit for events

tegefaulkes commented 2 months ago

This was addressed by the recent commit 0d7bc46663abfeb8dab0275fed412b0ca1384eec

CMCDragonkai commented 1 month ago

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 commented 1 month ago

Ok, I'll make the change.