Prevent NetworkConnections from leaking#8079
Conversation
🦋 Changeset detectedLatest commit: 5bdee7f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
alcuadrado
force-pushed
the
cache-genesis-state
branch
from
80d70dc to
437adf2
Compare
alcuadrado
force-pushed
the
memory-optimization
branch
from
f02c38e to
3ee3fdd
Compare
alcuadrado
added
no docs needed
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent network/provider objects from being retained longer than necessary by cleaning up EventEmitter listeners on close and breaking a potential reference cycle in the EDR subscription callback path.
Changes:
- Remove all EventEmitter listeners when closing
HttpProvider. - Use a
WeakRefforEdrProviderin the EDRsubscriptionCallbackto avoid strongly retaining the provider. - Remove all EventEmitter listeners when closing
EdrProvider.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| v-next/hardhat/src/internal/builtin-plugins/network-manager/http-provider.ts | Clears listeners during shutdown to reduce retention/leaks tied to provider events. |
| v-next/hardhat/src/internal/builtin-plugins/network-manager/edr/edr-provider.ts | Breaks subscription callback strong reference cycle and clears listeners on close. |
| .changeset/wild-pets-change.md | Adds a patch changeset describing the leak-related optimization. |
Comments suppressed due to low confidence (1)
v-next/hardhat/src/internal/builtin-plugins/network-manager/http-provider.ts:169
close()removes listeners before attempting to close the dispatcher and only sets#dispatchertoundefinedafterawait this.#dispatcher.close(). Ifdispatcher.close()rejects, the provider remains usable (because#dispatcheris still set) even thoughclose()was called, which can lead to confusing behavior and resource leaks. Consider ensuring the provider is marked closed regardless (e.g., set#dispatcher = undefinedin afinallyblock, or before awaitingclose()).
this.removeAllListeners();
if (this.#dispatcher !== undefined) {
// See https://github.com/nodejs/undici/discussions/3522#discussioncomment-10498734
await this.#dispatcher.close();
this.#dispatcher = undefined;
}
v-next/hardhat/src/internal/builtin-plugins/network-manager/http-provider.ts
Show resolved
Hide resolved
| subscriptionCallback: (event: SubscriptionEvent) => { | ||
| edrProvider.onSubscriptionEvent(event); | ||
| const deferredProvider = edrProviderWeakRef.deref(); | ||
| if (deferredProvider !== undefined) { | ||
| deferredProvider.onSubscriptionEvent(event); | ||
| } |
There was a problem hiding this comment.
edrProviderWeakRef is assigned only after createProvider(...) resolves, but it’s dereferenced unconditionally in subscriptionCallback. If the callback is invoked before edrProviderWeakRef is assigned (e.g., if the underlying provider emits an event during initialization), this will throw when calling .deref(). Consider making edrProviderWeakRef optional (WeakRef<EdrProvider> | undefined) and guarding before dereferencing.
v-next/hardhat/src/internal/builtin-plugins/network-manager/edr/edr-provider.ts
Show resolved
Hide resolved
kanej
left a comment
kanej
left a comment
There was a problem hiding this comment.
I have no points to add beyond co-pilot.
I tested this against OpenZeppelin and it passed - I have done no deeper profiling - but I suspect we will be doing multiple rounds of which this is the first.
alcuadrado
force-pushed
the
cache-genesis-state
branch
from
d50fbff to
ff71fb0
Compare
alcuadrado
force-pushed
the
memory-optimization
branch
from
8f8ca33 to
5bdee7f
Compare
No description provided.