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

Suggestion to simplify prevention of dangling Promise on destroy #286

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Suggestion to simplify prevention of dangling Promise on destroy #286

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3435c0a
Suggestion to simplify prevention of dangling Promise on destroy
Gudahtt Nov 20, 2024
83c5de0
Unset pending latest block after it has resolved
Gudahtt Nov 22, 2024
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
83 changes: 30 additions & 53 deletions src/PollingBlockTracker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import type { SafeEventEmitterProvider } from '@metamask/eth-json-rpc-provider';
import SafeEventEmitter from '@metamask/safe-event-emitter';
import { getErrorMessage, type JsonRpcRequest } from '@metamask/utils';
import {
createDeferredPromise,
getErrorMessage,
type JsonRpcRequest,
} from '@metamask/utils';
import getCreateRandomId from 'json-rpc-random-id';

import type { BlockTracker } from './BlockTracker';
Expand All @@ -26,7 +30,7 @@
skipCache?: boolean;
}

type InternalListener = (value: string | PromiseLike<string>) => void;
type InternalListener = (value: string) => void;

export class PollingBlockTracker
extends SafeEventEmitter
Expand Down Expand Up @@ -56,6 +60,10 @@

readonly #internalEventListeners: InternalListener[] = [];

#pendingLatestBlock:
| { promise: Promise<string>; reject: (error: unknown) => void }
| undefined;

constructor(opts: PollingBlockTrackerOptions = {}) {
// parse + validate args
if (!opts.provider) {
Expand Down Expand Up @@ -91,19 +99,10 @@
async destroy() {
this._cancelBlockResetTimeout();
this._maybeEnd();
this.eventNames().forEach((eventName) =>
this.listeners(eventName).forEach((listener) => {
if (
this.#internalEventListeners.every(
(internalListener) => !Object.is(internalListener, listener),
)
) {
// @ts-expect-error this listener comes from SafeEventEmitter itself, though
// its type differs between `.listeners()` and `.removeListener()`
this.removeListener(eventName, listener);
}
}),
);
this.removeAllListeners();
if (this.#pendingLatestBlock) {
this.#pendingLatestBlock.reject(new Error('Block tracker destroeyd'));
}
}

isRunning(): boolean {
Expand All @@ -118,37 +117,24 @@
// return if available
if (this._currentBlock) {
return this._currentBlock;
} else if (this.#pendingLatestBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good catch here. I was thinking of also using a deferred promise and keeping it in a private field, but it seemed that calling getLatestBlock multiple times without waiting would overwrite that field. This is a good solution.

return await this.#pendingLatestBlock.promise;
}
// wait for a new latest block
const latestBlock: string = await new Promise((resolve, reject) => {
// eslint-disable-next-line prefer-const
let onLatestBlockUnavailable: InternalListener;
const onLatestBlockAvailable = (value: string | PromiseLike<string>) => {
this.#removeInternalListener(onLatestBlockAvailable);
this.#removeInternalListener(onLatestBlockUnavailable);
this.removeListener('error', onLatestBlockUnavailable);
resolve(value);
};
onLatestBlockUnavailable = () => {
// if the block tracker is no longer running, reject
// and remove the listeners
if (!this._isRunning) {
this.#removeInternalListener(onLatestBlockAvailable);
this.#removeInternalListener(onLatestBlockUnavailable);
this.removeListener('latest', onLatestBlockAvailable);
this.removeListener('error', onLatestBlockUnavailable);
reject(
new Error('Block tracker ended before latest block was available'),
);
}
};
this.#addInternalListener(onLatestBlockAvailable);
this.#addInternalListener(onLatestBlockUnavailable);
this.once('latest', onLatestBlockAvailable);
this.on('error', onLatestBlockUnavailable);

const { promise, resolve, reject } = createDeferredPromise<string>({
suppressUnhandledRejection: true,
});
// return newly set current block
return latestBlock;
this.#pendingLatestBlock = { promise, reject };
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering, if we are making this promise available to the rest of the class, would it make sense to also resolve it through this class variable and get rid of the internal listener altogether?

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

True, yeah maybe so, good idea


// wait for a new latest block
const onLatestBlock = (value: string) => {
this.#removeInternalListener(onLatestBlock);
resolve(value);
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
delete this.#pendingLatestBlock;

Check failure on line 133 in src/PollingBlockTracker.ts

View workflow job for this annotation

GitHub Actions / Build, Lint, and Test (20.x) 

The operand of a 'delete' operator cannot be a private identifier.
};
this.#addInternalListener(onLatestBlock);
this.once('latest', onLatestBlock);
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
return await promise;
}

// dont allow module consumer to remove our internal event listeners
Expand Down Expand Up @@ -345,15 +331,6 @@

try {
this.emit('error', newErr);
if (
this.listeners('error').filter((listener) =>
this.#internalEventListeners.every(
(internalListener) => !Object.is(listener, internalListener),
),
).length === 0
) {
console.error(newErr);
}
} catch (emitErr) {
console.error(newErr);
}
Expand Down
Loading