Preventing MaxListenersExceededWarning #4421
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
suggested changes
Aug 16, 2022
| // if any before setting a new one. | ||
| if (this.emitter.listenerCount(ValidatorEvent.chainHead) > 0) { | ||
| this.emitter.off(ValidatorEvent.chainHead, headListener); | ||
| } | ||
| this.emitter.on(ValidatorEvent.chainHead, headListener); |
There was a problem hiding this comment.
This solution looks very strange, I don't follow the argument. The handler headListener is created in this scope so it can be proven that headListener has never been subscribed to this event, it's impossible. So the line this.emitter.off(ValidatorEvent.chainHead, headListener) is useless.
I also want see first a hard proof that the MaxListenersExceededWarning comes actually from this specific listener, and not from sleep()
There was a problem hiding this comment.
The handler headListener is created in this scope so it can be proven that headListener has never been subscribed to this event, it's impossible
In the case where waitForBlockSlot is called multiple times (before the unsubscribe), it is possible to have headListener subscribed multiple times. The intention of the check is to prevent this. But I will look into this again to ascertain your concern.
I also want see first a hard proof that the MaxListenersExceededWarning comes actually from this specific listener, and not from sleep()
I am almost certain it does not come from the sleep as no where in the sleep code path is EventEmitter used. The sleep code path makes use of AbortSignal and from what I see, that uses a different API. But will try and reproduce and confirm this.
dapplion
reviewed
Aug 23, 2022
| // We set infinity to prevent MaxListenersExceededWarning which get logged when listeners > 10 | ||
| // Since it is perfectly fine to have listeners > 10 | ||
| emitter.setMaxListeners(Infinity); | ||
| setMaxListeners(Infinity, signal); |
There was a problem hiding this comment.
Can you commit as comment the stack trace proving that this very specific event emitter gets more that 10 listeners?
There was a problem hiding this comment.
I could not create a situation where the warning is triggered locally. So I have removed the changes.
dapplion
reviewed
Aug 23, 2022
dapplion
reviewed
Aug 23, 2022
| @@ -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); | |||
There was a problem hiding this comment.
Can you commit as comment the stack trace proving that this very specific event emitter gets more that 10 listeners?
There was a problem hiding this comment.
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
dapplion
reviewed
Aug 23, 2022
| @@ -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)]); | |||
There was a problem hiding this comment.
Why is it necessary to change the order here?
There was a problem hiding this comment.
It was not a necessity. It was changed to align with the text of the spec, which was placed as a comment
There was a problem hiding this comment.
What do you mean the spec? Where does the spec specify the order of promises in javascript?
dapplion
reviewed
Aug 23, 2022
| @@ -41,6 +42,10 @@ export async function beaconHandler(args: IBeaconArgs & IGlobalArgs): Promise<vo | |||
| const options = beaconNodeOptions.getWithDefaults(); | |||
|
|
|||
| const abortController = 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, abortController.signal); | |||
There was a problem hiding this comment.
Can you commit as comment the stack trace proving that this very specific event emitter gets more that 10 listeners?
There was a problem hiding this comment.
I could not create a situation where the warning is triggered locally. So I have removed the changes
dapplion
reviewed
Aug 23, 2022
packages/validator/src/validator.ts
Outdated
| @@ -85,6 +86,10 @@ export class Validator { | |||
| } = opts; | |||
| const config = createIBeaconConfig(dbOps.config, genesis.genesisValidatorsRoot); | |||
| this.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, this.controller.signal); | |||
There was a problem hiding this comment.
Can you commit as comment the stack trace proving that this very specific event emitter gets more that 10 listeners? Same for below
There was a problem hiding this comment.
I removed the changes for the abort controller, as I could find a situation where it triggered the warning. I left the one for ValidatorEventEmitter, even though I could not recreate the situation that triggered it, I did observe by chance back in January. See #3569. The log reproduced here
(node:15159) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 chainHead listeners added to [ValidatorEventEmitter]. Use emitter.setMaxListeners() to increase limit
dadepo
commented
Aug 24, 2022
| @@ -71,6 +72,9 @@ export class ReqResp implements IReqResp { | |||
|
|
|||
| async start(): Promise<void> { | |||
| this.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, this.controller.signal); | |||
There was a problem hiding this comment.
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
dapplion
reviewed
Aug 26, 2022
| private reqRespHandlers: ReqRespHandlers; | ||
| private metadataController: MetadataController; | ||
| private peerRpcScores: IPeerRpcScoreStore; | ||
| private inboundRateLimiter: IRateLimiter; | ||
| private networkEventBus: INetworkEventBus; | ||
| private controller = new AbortController(); |
There was a problem hiding this comment.
Why do you need to initialize the AbortController on constructor?
There was a problem hiding this comment.
Not sure why this is showing as a diff here, but this is not newly introduced. See it on unstable branch. See here
dapplion
reviewed
Aug 26, 2022
| @@ -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); | |||
There was a problem hiding this comment.
Can you add proof with stack trace that this event listener triggers warnings?
There was a problem hiding this comment.
Already responded to the diff in the comment here
dapplion
approved these changes
Sep 1, 2022
wemeetagain
approved these changes
Sep 1, 2022
| @@ -1,3 +1,4 @@ | |||
| import {setMaxListeners} from "node:events"; | |||
There was a problem hiding this comment.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
See #3569 and # #4394 for bug report
Description
The waitForBlockSlot should execute once per slot and per slot there should be a ValidatorEvent.chainHead event emitted In the unexpected situation where ValidatorEvent.chainHead is never emitted or is emitted but takes longer to propagate to the validator, to prevent accumulating listeners for ValidatorEvent.chainHead and triggering MaxListenersExceededWarning.
This PR clears listener for ValidatorEvent.chainHead if any before setting a new one.
Closes #3569
Beacon Node
Validator Client
Spent some time trying to reproducing the warning for the validator client but could not. Even though back in January I observed the warning emanating from
ValidatorEventEmittersee #3569