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.

Sign up for GitHub

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

Jump to bottom

Preventing MaxListenersExceededWarning #4421

Merged
merged 6 commits into from
Sep 1, 2022
Merged

Preventing MaxListenersExceededWarning #4421

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ebdbb50
preventing MaxListenersExceededWarning
dadepo Aug 15, 2022
7802a2a
Merge branch 'unstable' into dadepo/fix-eventemitter-memory-log
dadepo Aug 23, 2022
d86bb68
set setMaxListeners to Infinity
dadepo Aug 23, 2022
72f211f
Removing setting max listeners to infinity in cases where it has not …
dadepo Aug 24, 2022
ccd11c1
set max listeners for ValidatorEventEmitter to infinity
dadepo Aug 24, 2022
ea7ef2f
revert ordering of promise race
dadepo Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/beacon-node/src/network/reqresp/reqResp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {setMaxListeners} from "node:events";
Copy link
Member

Choose a reason for hiding this comment

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

Shame we have to introduce a nodejs specific api here.

Later we can pass down a signal from the top-level beacon node and avoid creating a new signal here.

import {Connection} from "libp2p";
import {HandlerProps} from "libp2p/src/registrar";
import LibP2p from "libp2p";
Expand Down Expand Up @@ -71,6 +72,9 @@ export class ReqResp implements IReqResp {

async start(): Promise<void> {
this.controller = new AbortController();
dapplion marked this conversation as resolved.
Show resolved Hide resolved
// We set infinity to prevent MaxListenersExceededWarning which get logged when listeners > 10
// Since it is perfectly fine to have listeners > 10
setMaxListeners(Infinity, this.controller.signal);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responsible for the warning observed in the trace:

(node:56301) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
    at AbortSignal.[kNewListener] (node:internal/event_target:426:17)
    at AbortSignal.[kNewListener] (node:internal/abort_controller:173:24)
    at AbortSignal.addEventListener (node:internal/event_target:535:23)
    at anySignal (/Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/node_modules/any-signal/index.js:27:12)
    at withTimeout (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/utils/src/timeout.ts:11:34)
    at sendRequest (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/network/reqresp/request/index.ts:87:50)
    at ReqResp.sendRequest (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/network/reqresp/reqResp.ts:154:28)
    at ReqResp.status (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/network/reqresp/reqResp.ts:92:23)
    at PeerManager.requestStatus (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/network/peers/peerManager.ts:359:46)
    at file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/network/peers/peerManager.ts:368:56

for (const [method, version, encoding] of protocolsSupported) {
await this.libp2p.handle(
formatProtocolId(method, version, encoding),
Expand Down
4 changes: 4 additions & 0 deletions packages/beacon-node/src/node/nodejs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {setMaxListeners} from "node:events";
import LibP2p from "libp2p";
import {Registry} from "prom-client";

Expand Down Expand Up @@ -118,6 +119,9 @@ export class BeaconNode {
metricsRegistries = [],
}: IBeaconNodeInitModules): Promise<T> {
const controller = new AbortController();
// We set infinity to prevent MaxListenersExceededWarning which get logged when listeners > 10
// Since it is perfectly fine to have listeners > 10
setMaxListeners(Infinity, controller.signal);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you commit as comment the stack trace proving that this very specific event emitter gets more that 10 listeners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The signal here was what was passed down and triggered this:

(node:56301) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
    at AbortSignal.[kNewListener] (node:internal/event_target:426:17)
    at AbortSignal.[kNewListener] (node:internal/abort_controller:173:24)
    at AbortSignal.addEventListener (node:internal/event_target:535:23)
    at JsonRpcHttpClient.fetchJsonOneUrl (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts:194:24)
    at JsonRpcHttpClient.fetchJson (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts:160:27)
    at retry.retries._b (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts:125:27)
    at retry (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/utils/src/retry.ts:33:20)
    at JsonRpcHttpClient.fetchWithRetries (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts:119:23)
    at ExecutionEngineHttp.exchangeTransitionConfigurationV1 (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/execution/engine/http.ts:298:27)
    at BeaconChain.exchangeTransitionConfiguration (file:///Users/dadepoaderemi/Documents/work/chainsafe/official/lodestar/packages/beacon-node/src/chain/chain.ts:573:62)
Aug-17 21:32:00.064[CHAI

const signal = controller.signal;

// start db if not already started
Expand Down
2 changes: 1 addition & 1 deletion packages/validator/src/services/attestation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export class AttestationService {
// A validator should create and broadcast the attestation to the associated attestation subnet when either
// (a) the validator has received a valid block from the expected block proposer for the assigned slot or
// (b) one-third of the slot has transpired (SECONDS_PER_SLOT / 3 seconds after the start of slot) -- whichever comes first.
await Promise.race([sleep(this.clock.msToSlot(slot + 1 / 3), signal), this.waitForBlockSlot(slot)]);
await Promise.race([this.waitForBlockSlot(slot), sleep(this.clock.msToSlot(slot + 1 / 3), signal)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it necessary to change the order here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not a necessity. It was changed to align with the text of the spec, which was placed as a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean the spec? Where does the spec specify the order of promises in javascript?

this.metrics?.attesterStepCallProduceAttestation.observe(this.clock.secFromSlot(slot + 1 / 3));

// Beacon node's endpoint produceAttestationData return data is not dependant on committeeIndex.
Expand Down
4 changes: 4 additions & 0 deletions packages/validator/src/validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ export class Validator {
}

const emitter = new ValidatorEventEmitter();
// Validator event emitter can have more than 10 listeners in a normal course of operation
// We set infinity to prevent MaxListenersExceededWarning which get logged when listeners > 10
emitter.setMaxListeners(Infinity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add proof with stack trace that this event listener triggers warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already responded to the diff in the comment here


const chainHeaderTracker = new ChainHeaderTracker(logger, api, emitter);

this.blockProposingService = new BlockProposingService(config, loggerVc, api, clock, validatorStore, metrics, {
Expand Down