fix: EventEmitter memory leak - #39
Conversation
| }); | ||
|
|
||
| // bind listener to Derived class instance | ||
| this.coordinator.emitter.on('metadata_updated', async ({ id, metadata }) => { |
There was a problem hiding this comment.
This listener bound on every Credential instance, but filter to only fire on the specific Credential. It's incorrectly space in method space. Moved this logic to static section where other listeners are bound
| this.emitter.emit('credential_removed', { id }); | ||
| }); | ||
|
|
||
| this.coordinator.emitter.on('metadata_updated', async ({ id, metadata }) => { |
There was a problem hiding this comment.
The tags_updated trigger moved from observeToken
| /** | ||
| * Cleans up resourece associated with the Credential instance to prevent leaks. | ||
| */ | ||
| public dispose () { |
There was a problem hiding this comment.
Added a method to clean resources used by Credential instances. Since the tags_updated listener was moved to static space, the only instance-level listener is token_did_refresh of the OAuth2 instance with Credential. oauth2.dispose clears that listener.
Also added an AbortController instance of each Credential for future proofing of clearing resources
There was a problem hiding this comment.
The pattern of clearing this.oauth2.emitter listeners also solves this nested listener which contributed to the memory leak
| const id = typeof cred === 'string' ? cred : cred.id; | ||
| if (this.credentials.has(id)) { | ||
| const cred = this.credentials.get(id)!; | ||
| cred.dispose(); |
There was a problem hiding this comment.
calls new .dipose method on Credentials during .remove()
| /** | ||
| * Cleans up resourece associated with the client instance to prevent leaks. | ||
| */ | ||
| public dispose () { |
There was a problem hiding this comment.
Used within Credential via OAuth2.dispose. Clears active listeners on the EventEmitter to prevent similar leaks
| /** | ||
| * Cleans up resourece associated with the client instance to prevent leaks. | ||
| */ | ||
| public dispose () { |
There was a problem hiding this comment.
Used within Credential.dispose. Clears active listeners on this.emitter via super.dispose to prevent similar leaks. Also clears the #httpCache
| } | ||
| this.listeners[eventName]!.push(handler); | ||
|
|
||
| if (signal && !signal?.aborted) { |
There was a problem hiding this comment.
This pattern isn't really utilized yet, but for future-proofing. Listens on a provided AbortSignal (presumably via an AbortController) and clears the event listener bound at the same time as the AbortSignal. Could useful to assist resource clean up of other entities in the future
| log('removal'); | ||
| this.credentialDataSource.remove(id); | ||
|
|
||
| // // removal messages never carried a token body; `id` is all `hasCredential` actually reads |
There was a problem hiding this comment.
Need to confirm with testing this section is no longer needed
| // @ts-expect-error - Credential `set token()` is a private setter to avoid exposing this to the public API | ||
| credential.token = token; | ||
| this.emitter.emit('credential_refreshed', { credential }); | ||
| const credential = this.credentialDataSource.credentialFor(token); |
There was a problem hiding this comment.
Per AI suggestion, there is probably no need to broadcast the token value when synchronizing tabs, each tab can access the token value from storage (the source of truth) to save on memory usage
1 participant
No description provided.