From 0638bf651a0302cbd88939f2b83c04bc0ab81be6 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Wed, 27 Jul 2022 15:50:02 +1200 Subject: [PATCH 01/14] Reject all pending requests, and stop processing requets once provider.disconnect() is called. --- src/chains/ethereum/ethereum/src/provider.ts | 2 +- .../ethereum/ethereum/tests/provider.test.ts | 10 ++++++++ src/packages/utils/src/utils/executor.ts | 4 ++++ .../utils/src/utils/request-coordinator.ts | 23 ++++++++++++++++++- 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index 62c9c2a7f6..6c6cab02d6 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -420,9 +420,9 @@ export class EthereumProvider }; public disconnect = async () => { + this.#executor.disconnect(); await this.#blockchain.stop(); this.emit("disconnect"); - return; }; //#region legacy diff --git a/src/chains/ethereum/ethereum/tests/provider.test.ts b/src/chains/ethereum/ethereum/tests/provider.test.ts index b8133ed6bf..27e6117517 100644 --- a/src/chains/ethereum/ethereum/tests/provider.test.ts +++ b/src/chains/ethereum/ethereum/tests/provider.test.ts @@ -423,6 +423,16 @@ describe("provider", () => { } ); }); + + it("stops responding to RPC methods once disconnected", async () => { + const provider = await getProvider(); + await provider.disconnect(); + + await assert.rejects( + provider.send("eth_getBlockByNumber", ["latest"]), + new Error("Cannot process request, Ganache is disconnected.") + ); + }); }); describe("web3 compatibility", () => { diff --git a/src/packages/utils/src/utils/executor.ts b/src/packages/utils/src/utils/executor.ts index cbb1beeb65..6521812160 100644 --- a/src/packages/utils/src/utils/executor.ts +++ b/src/packages/utils/src/utils/executor.ts @@ -12,6 +12,10 @@ export class Executor { this.#requestCoordinator = requestCoordinator; } + public disconnect() { + this.#requestCoordinator.disconnect(); + } + /** * Executes the method with the given methodName on the API * @param methodName - The name of the JSON-RPC method to execute. diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index 689d3de061..d04167094f 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -1,6 +1,9 @@ import { OverloadedParameters } from "../types"; const noop = () => {}; +type RejectableTask = ((...args: any) => Promise) & { + reject: (reason?: any) => void; +}; /** * Responsible for managing global concurrent requests. @@ -14,7 +17,7 @@ export class RequestCoordinator { /** * The pending requests. You can't do anything with this array. */ - public readonly pending: ((...args: any) => Promise)[] = []; + public readonly pending: RejectableTask[] = []; /** * The number of tasks currently being processed. @@ -74,6 +77,23 @@ export class RequestCoordinator { } }; + public disconnect() { + this.pause(); + // make this async to force a Promise return type + this.queue = async () => { + throw new Error("Cannot process request, Ganache is disconnected."); + }; + // ensure that processing cannot be resumed + this.resume = () => { + throw new Error("Cannot resume processing requests, Ganache is disconnected."); + } + + while (this.pending.length > 0) { + const current = this.pending.shift(); + current.reject(new Error("Cannot process request, Ganache is disconnected.")); + } + }; + /** * Insert a new function into the queue. */ @@ -97,6 +117,7 @@ export class RequestCoordinator { reject(e); } }; + executor.reject = reject; this.pending.push(executor); this.#process(); }); From ccafb7ec459b182b68fd932a772b43a8ee7d39d3 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Wed, 27 Jul 2022 15:59:57 +1200 Subject: [PATCH 02/14] Add the tests :/ --- .../utils/tests/request-coordinator.test.ts | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 src/packages/utils/tests/request-coordinator.test.ts diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts new file mode 100644 index 0000000000..a6bbeb4e39 --- /dev/null +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -0,0 +1,48 @@ +import assert from "assert"; +import { RequestCoordinator } from "../src/utils/request-coordinator"; + +describe("request-coordinator", () => { + let coordinator: RequestCoordinator; + + beforeEach("instantiate RequestCoordinator", () => { + coordinator = new RequestCoordinator(0); + }); + + describe("disconnect", () => { + it("should pause processing", () => { + coordinator.disconnect(); + + assert(coordinator.paused); + }); + + it("should not allow processing to be resumed", () => { + coordinator.disconnect(); + + assert.throws( + () => coordinator.resume(), + new Error("Cannot resume processing requests, Ganache is disconnected.") + ); + }); + + it("should not allow new requests to be queued", async () => { + coordinator.disconnect(); + + await assert.rejects( + coordinator.queue(() => null, this, []), + new Error("Cannot process request, Ganache is disconnected.") + ); + }); + + it("should reject all queued requests", async () => { + const neverEndingTask = () => { + return new Promise(() => {}); + }; + const taskPromise = coordinator.queue(neverEndingTask, this, []); + coordinator.disconnect(); + await assert.rejects( + taskPromise, + new Error("Cannot process request, Ganache is disconnected.") + ); + }); + }); +}); From 2528daa303dbd623f9ccdaacd04c6f08c92ce471 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Wed, 27 Jul 2022 22:11:25 +1200 Subject: [PATCH 03/14] Comments, and minor tidy up --- .../utils/src/utils/request-coordinator.ts | 19 +++++++++++++------ .../utils/tests/request-coordinator.test.ts | 5 +---- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index d04167094f..fdfad97080 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -79,20 +79,27 @@ export class RequestCoordinator { public disconnect() { this.pause(); - // make this async to force a Promise return type + // ensure nothing can be requeued (although tasks can be added directly to this.pending + // but they will never be processed). We make this async to force a Promise return type. this.queue = async () => { throw new Error("Cannot process request, Ganache is disconnected."); }; - // ensure that processing cannot be resumed + + // ensure that processing cannot be resumed. this.resume = () => { - throw new Error("Cannot resume processing requests, Ganache is disconnected."); - } + throw new Error( + "Cannot resume processing requests, Ganache is disconnected." + ); + }; + // purge any pending tasks, respecting FIFO nature of the queue while (this.pending.length > 0) { const current = this.pending.shift(); - current.reject(new Error("Cannot process request, Ganache is disconnected.")); + current.reject( + new Error("Cannot process request, Ganache is disconnected.") + ); } - }; + } /** * Insert a new function into the queue. diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index a6bbeb4e39..c29df15a3c 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -34,10 +34,7 @@ describe("request-coordinator", () => { }); it("should reject all queued requests", async () => { - const neverEndingTask = () => { - return new Promise(() => {}); - }; - const taskPromise = coordinator.queue(neverEndingTask, this, []); + const taskPromise = coordinator.queue(() => null, this, []); coordinator.disconnect(); await assert.rejects( taskPromise, From b1327ed196fac6e25cb1e1662e95ea99442b2c73 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Mon, 1 Aug 2022 14:21:15 +1200 Subject: [PATCH 04/14] Await blockchain.stop before rejecting pending tasks, --- src/chains/ethereum/ethereum/src/provider.ts | 18 +++++-- src/packages/utils/src/utils/executor.ts | 9 +++- .../utils/src/utils/request-coordinator.ts | 34 ++++++++---- .../utils/tests/request-coordinator.test.ts | 53 ++++++++++++++----- 4 files changed, 86 insertions(+), 28 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index 6c6cab02d6..9d67208e28 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -35,7 +35,6 @@ import { MessageEvent, VmConsoleLogEvent } from "./provider-events"; -import { ConsoleLogs } from "@ganache/console.log"; declare type RequestMethods = KnownKeys; @@ -141,8 +140,8 @@ export class EthereumProvider { #options: EthereumInternalOptions; #api: EthereumApi; - #executor: Executor; #wallet: Wallet; + readonly #executor: Executor; readonly #blockchain: Blockchain; constructor( @@ -419,9 +418,22 @@ export class EthereumProvider } }; + /** + * Disconnect the provider instance. This will cause the underlying blockchain to be stopped, and any pending + * tasks to be rejected. Await the returned Promise to ensure that everything has been cleanly shut down + * before terminating the process. + * @return Promise - indicating that the provider has been cleanly disconnected + */ public disconnect = async () => { - this.#executor.disconnect(); + const coordinator = this.#executor.getCoordinator(); + + // We make a best effort to resolve any currently executing tasks, before rejecting pending tasks. This relies on + // this.#blockchain.stop() waiting to resolve until after all executing tasks have settled. Executor does not + // guarantee that no tasks are currently executing, before rejecting any remaining pending tasks. + coordinator.stop(); await this.#blockchain.stop(); + coordinator.rejectPendingTasks(); + this.emit("disconnect"); }; diff --git a/src/packages/utils/src/utils/executor.ts b/src/packages/utils/src/utils/executor.ts index 6521812160..76923a0fe1 100644 --- a/src/packages/utils/src/utils/executor.ts +++ b/src/packages/utils/src/utils/executor.ts @@ -12,10 +12,15 @@ export class Executor { this.#requestCoordinator = requestCoordinator; } - public disconnect() { - this.#requestCoordinator.disconnect(); + public stop() { + this.#requestCoordinator.stop(); } + public rejectPendingTasks() { + this.#requestCoordinator.rejectPendingTasks(); + } + + /** * Executes the method with the given methodName on the API * @param methodName - The name of the JSON-RPC method to execute. diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index fdfad97080..f7fb58afce 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -1,7 +1,8 @@ import { OverloadedParameters } from "../types"; const noop = () => {}; -type RejectableTask = ((...args: any) => Promise) & { +type RejectableTask = { + execute: (...args: any) => Promise; reject: (reason?: any) => void; }; @@ -63,7 +64,8 @@ export class RequestCoordinator { ) { const current = this.pending.shift(); this.runningTasks++; - current() + current + .execute() // By now, we've resolved the fn's `value` by sending it to the parent scope. // But over here, we're also waiting for this fn's _value_ to settle _itself_ (it might be a promise) before // continuing through the `pending` queue. Because we wait for it again here, it could potentially throw here, @@ -77,10 +79,17 @@ export class RequestCoordinator { } }; - public disconnect() { + /** + * Stop processing tasks - calls to queue(), and resume() will reject with an error indicating that Ganache is + * disconnected. This is an irreversible action. If you wish to be able to resume processing, use pause() instead. + * + * Note: This will _not_ reject any pending tasks - see rejectPendingTasks() + */ + public stop() { this.pause(); - // ensure nothing can be requeued (although tasks can be added directly to this.pending - // but they will never be processed). We make this async to force a Promise return type. + + // ensure nothing can be requeued (although tasks can be added directly to this.pending but they will never be + // processed). We make this async to force a Promise return type. this.queue = async () => { throw new Error("Cannot process request, Ganache is disconnected."); }; @@ -91,10 +100,14 @@ export class RequestCoordinator { "Cannot resume processing requests, Ganache is disconnected." ); }; + } - // purge any pending tasks, respecting FIFO nature of the queue - while (this.pending.length > 0) { - const current = this.pending.shift(); + /** + * Reject any pending tasks with an error indicating that Ganache is disconnected. Tasks are rejected in FIFO order. + */ + public rejectPendingTasks() { + let current: RejectableTask; + while(current = this.pending.shift()) { current.reject( new Error("Cannot process request, Ganache is disconnected.") ); @@ -111,7 +124,7 @@ export class RequestCoordinator { ) => { return new Promise<{ value: ReturnType }>((resolve, reject) => { // const executor is `async` to force the return value into a Promise. - const executor = async () => { + const execute = async () => { try { const value = Reflect.apply( fn, @@ -124,8 +137,7 @@ export class RequestCoordinator { reject(e); } }; - executor.reject = reject; - this.pending.push(executor); + this.pending.push({ execute, reject }); this.#process(); }); }; diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index c29df15a3c..a1b92e4673 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -2,21 +2,22 @@ import assert from "assert"; import { RequestCoordinator } from "../src/utils/request-coordinator"; describe("request-coordinator", () => { + const noop = () => undefined; let coordinator: RequestCoordinator; beforeEach("instantiate RequestCoordinator", () => { coordinator = new RequestCoordinator(0); }); - describe("disconnect", () => { + describe("stop()", () => { it("should pause processing", () => { - coordinator.disconnect(); + coordinator.stop(); assert(coordinator.paused); }); it("should not allow processing to be resumed", () => { - coordinator.disconnect(); + coordinator.stop(); assert.throws( () => coordinator.resume(), @@ -25,21 +26,49 @@ describe("request-coordinator", () => { }); it("should not allow new requests to be queued", async () => { - coordinator.disconnect(); + coordinator.stop(); await assert.rejects( - coordinator.queue(() => null, this, []), + coordinator.queue(noop, this, []), new Error("Cannot process request, Ganache is disconnected.") ); }); + }); - it("should reject all queued requests", async () => { - const taskPromise = coordinator.queue(() => null, this, []); - coordinator.disconnect(); - await assert.rejects( - taskPromise, - new Error("Cannot process request, Ganache is disconnected.") - ); + describe("rejectAllPendingRequests()", () => { + it("should reject pending requests in the order that they were received", async () => { + coordinator.pause(); + + let taskIndex = 0; + const pendingAssertions: Promise[] = []; + for (let i = 0; i < 10; i++) { + const task = coordinator.queue(noop, this, []); + + let nextRejectionIndex = taskIndex; + pendingAssertions.push(task.catch(_ => { + assert.strictEqual(i, nextRejectionIndex, `Rejected in incorrect order, waiting on task at index ${nextRejectionIndex}, got ${i}.`); + })); + + taskIndex++; + + pendingAssertions.push(assert.rejects(task, new Error("Cannot process request, Ganache is disconnected."))); + } + + coordinator.rejectPendingTasks(); + await Promise.all(pendingAssertions); + }); + + it("should clear the pending tasks queue", () => { + coordinator.pause(); + + for (let i = 0; i < 10; i++) { + coordinator.queue(noop, this, []); + } + + assert.equal(coordinator.pending.length, 10, "Incorrect pending queue length before calling rejectAllPendingRequests"); + + coordinator.rejectPendingTasks(); + assert.equal(coordinator.pending.length, 0, "Incorrect pending queue length after calling rejectAllPendingRequests"); }); }); }); From e47d3ed2f4bc2877dc57e7c6343232c0fb7a8cdb Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Mon, 1 Aug 2022 14:25:33 +1200 Subject: [PATCH 05/14] oops - still calling .getCoordinator(), even after removing it --- src/chains/ethereum/ethereum/src/provider.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index 9d67208e28..c93497159b 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -425,14 +425,13 @@ export class EthereumProvider * @return Promise - indicating that the provider has been cleanly disconnected */ public disconnect = async () => { - const coordinator = this.#executor.getCoordinator(); - + const executor = this.#executor; // We make a best effort to resolve any currently executing tasks, before rejecting pending tasks. This relies on // this.#blockchain.stop() waiting to resolve until after all executing tasks have settled. Executor does not // guarantee that no tasks are currently executing, before rejecting any remaining pending tasks. - coordinator.stop(); + executor.stop(); await this.#blockchain.stop(); - coordinator.rejectPendingTasks(); + executor.rejectPendingTasks(); this.emit("disconnect"); }; From 01d62f83f8058dae0aae269ada023b2f8713ad17 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Wed, 3 Aug 2022 17:00:44 +1200 Subject: [PATCH 06/14] WIP - ensure that in progress tasks complete before stopping the blockchain. --- src/chains/ethereum/ethereum/src/provider.ts | 6 +- src/packages/utils/src/utils/executor.ts | 4 +- .../utils/src/utils/request-coordinator.ts | 54 ++++++++---- .../utils/tests/request-coordinator.test.ts | 82 +++++++++++++++---- 4 files changed, 110 insertions(+), 36 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index c93497159b..7a3d1cdd8d 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -429,9 +429,11 @@ export class EthereumProvider // We make a best effort to resolve any currently executing tasks, before rejecting pending tasks. This relies on // this.#blockchain.stop() waiting to resolve until after all executing tasks have settled. Executor does not // guarantee that no tasks are currently executing, before rejecting any remaining pending tasks. - executor.stop(); - await this.#blockchain.stop(); + + // await executor.stop() to ensure that all currently processing tasks are complete + await executor.stop(); executor.rejectPendingTasks(); + await this.#blockchain.stop(); this.emit("disconnect"); }; diff --git a/src/packages/utils/src/utils/executor.ts b/src/packages/utils/src/utils/executor.ts index 76923a0fe1..520325c1f1 100644 --- a/src/packages/utils/src/utils/executor.ts +++ b/src/packages/utils/src/utils/executor.ts @@ -12,8 +12,8 @@ export class Executor { this.#requestCoordinator = requestCoordinator; } - public stop() { - this.#requestCoordinator.stop(); + public stop(): Promise { + return this.#requestCoordinator.stop(); } public rejectPendingTasks() { diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index f7fb58afce..73c055fbbd 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -24,7 +24,10 @@ export class RequestCoordinator { * The number of tasks currently being processed. */ public runningTasks: number = 0; - #paused: boolean = true; + + #paused = true; + #stopped = false; + public get paused(): boolean { return this.#paused; } @@ -50,6 +53,12 @@ export class RequestCoordinator { * Resume processing. */ public resume = () => { + if (this.#stopped) { + throw new Error( + "Cannot resume processing requests, Ganache is disconnected." + ); + } + this.#paused = false; this.#process(); }; @@ -74,32 +83,40 @@ export class RequestCoordinator { .catch(noop) .finally(() => { this.runningTasks--; - this.#process(); + + if (this.runningTasks === 0 && this.pending.length === 0) { + this.workComplete(); + } else { + this.#process(); + } }); } }; + #whenFinished: () => void; + private workComplete() { + if (this.#whenFinished) { + this.#whenFinished(); + this.#whenFinished = undefined; + } + } + /** * Stop processing tasks - calls to queue(), and resume() will reject with an error indicating that Ganache is * disconnected. This is an irreversible action. If you wish to be able to resume processing, use pause() instead. * * Note: This will _not_ reject any pending tasks - see rejectPendingTasks() + * @returns Promise - indicating that all currently executing tasks are complete */ - public stop() { + public async stop(): Promise { this.pause(); + this.#stopped = true; - // ensure nothing can be requeued (although tasks can be added directly to this.pending but they will never be - // processed). We make this async to force a Promise return type. - this.queue = async () => { - throw new Error("Cannot process request, Ganache is disconnected."); - }; - - // ensure that processing cannot be resumed. - this.resume = () => { - throw new Error( - "Cannot resume processing requests, Ganache is disconnected." - ); - }; + if (this.runningTasks > 0) { + await new Promise((resolve, _) => { + this.#whenFinished = resolve; + }); + } } /** @@ -107,7 +124,7 @@ export class RequestCoordinator { */ public rejectPendingTasks() { let current: RejectableTask; - while(current = this.pending.shift()) { + while ((current = this.pending.shift())) { current.reject( new Error("Cannot process request, Ganache is disconnected.") ); @@ -122,6 +139,11 @@ export class RequestCoordinator { thisArgument: any, argumentsList: OverloadedParameters ) => { + if (this.#stopped) { + return Promise.reject( + new Error("Cannot process request, Ganache is disconnected.") + ); + } return new Promise<{ value: ReturnType }>((resolve, reject) => { // const executor is `async` to force the return value into a Promise. const execute = async () => { diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index a1b92e4673..cb0c1ca8f0 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -33,6 +33,52 @@ describe("request-coordinator", () => { new Error("Cannot process request, Ganache is disconnected.") ); }); + + it("should stop when tasks are queued", async () => { + const uncompletable = coordinator.queue(() => new Promise(noop), this, []); + await coordinator.stop(); + + assert(coordinator.paused); + }); + + describe("should wait for in-flight tasks to complete before resolving", () => { + it("when the task resolves", async () => { + const inProgressTask = coordinator.queue(noop, this, []); + coordinator.resume(); + + let isStopped = false; + const stopped = coordinator.stop().then(() => (isStopped = true)); + + await inProgressTask; + assert( + !isStopped, + "return result of RequestCoordinator.stop() resolved before the pending task completed" + ); + + await assert.doesNotReject(stopped); + }); + + it("when the task rejects", async () => { + const inProgressTask = coordinator.queue( + () => Promise.reject(), + this, + [] + ); + coordinator.resume(); + + let isStopped = false; + const stopped = coordinator.stop().then(() => (isStopped = true)); + + // the promise returned from coordinator.queue will resolve, even though the underlying promise rejects + await inProgressTask; + assert( + !isStopped, + "return result of RequestCoordinator.stop() resolved before the pending task completed" + ); + + await assert.doesNotReject(stopped); + }); + }); }); describe("rejectAllPendingRequests()", () => { @@ -45,30 +91,34 @@ describe("request-coordinator", () => { const task = coordinator.queue(noop, this, []); let nextRejectionIndex = taskIndex; - pendingAssertions.push(task.catch(_ => { - assert.strictEqual(i, nextRejectionIndex, `Rejected in incorrect order, waiting on task at index ${nextRejectionIndex}, got ${i}.`); - })); + pendingAssertions.push( + task.catch(_ => { + assert.strictEqual( + i, + nextRejectionIndex, + `Rejected in incorrect order, waiting on task at index ${nextRejectionIndex}, got ${i}.` + ); + }) + ); taskIndex++; - pendingAssertions.push(assert.rejects(task, new Error("Cannot process request, Ganache is disconnected."))); + pendingAssertions.push( + assert.rejects( + task, + new Error("Cannot process request, Ganache is disconnected.") + ) + ); } coordinator.rejectPendingTasks(); await Promise.all(pendingAssertions); - }); - it("should clear the pending tasks queue", () => { - coordinator.pause(); - - for (let i = 0; i < 10; i++) { - coordinator.queue(noop, this, []); - } - - assert.equal(coordinator.pending.length, 10, "Incorrect pending queue length before calling rejectAllPendingRequests"); - - coordinator.rejectPendingTasks(); - assert.equal(coordinator.pending.length, 0, "Incorrect pending queue length after calling rejectAllPendingRequests"); + assert.equal( + coordinator.pending.length, + 0, + "Incorrect pending queue length after calling rejectAllPendingRequests" + ); }); }); }); From a59b00d933d091632335016b87f7e0f768279ddf Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Thu, 4 Aug 2022 15:15:45 +1200 Subject: [PATCH 07/14] Tidy up request-coordinator and add tests at EthereumProvider level --- src/chains/ethereum/ethereum/src/provider.ts | 23 ++-- .../ethereum/ethereum/tests/provider.test.ts | 110 ++++++++++++++++-- .../utils/src/utils/request-coordinator.ts | 42 +++---- .../utils/tests/request-coordinator.test.ts | 7 +- 4 files changed, 140 insertions(+), 42 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index 7a3d1cdd8d..5b645ef61f 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -138,9 +138,9 @@ export class EthereumProvider }> implements Provider { - #options: EthereumInternalOptions; - #api: EthereumApi; - #wallet: Wallet; + readonly #options: EthereumInternalOptions; + readonly #api: EthereumApi; + readonly #wallet: Wallet; readonly #executor: Executor; readonly #blockchain: Blockchain; @@ -419,19 +419,20 @@ export class EthereumProvider }; /** - * Disconnect the provider instance. This will cause the underlying blockchain to be stopped, and any pending - * tasks to be rejected. Await the returned Promise to ensure that everything has been cleanly shut down - * before terminating the process. - * @return Promise - indicating that the provider has been cleanly disconnected + * Disconnect the provider, the underlying blockchain will be stopped. Any tasks currently executing will be completed, + * any pending tasks will be rejected. The returned Promise will resolve when the provider is disconnected, and a + * "disconnect" event will be emitted. + * @returns Promise - resolves when the provider has disconnected */ public disconnect = async () => { const executor = this.#executor; - // We make a best effort to resolve any currently executing tasks, before rejecting pending tasks. This relies on - // this.#blockchain.stop() waiting to resolve until after all executing tasks have settled. Executor does not - // guarantee that no tasks are currently executing, before rejecting any remaining pending tasks. - // await executor.stop() to ensure that all currently processing tasks are complete + // we await executor.stop() here to ensure that any currently executing tasks are complete before pulling the + // rug out by stopping the blockchain. await executor.stop(); + + // we call rejectPendingTasks() _after_ executor.stop() has resolved, to ensure that all tasks are executed in + // FIFO order executor.rejectPendingTasks(); await this.#blockchain.stop(); diff --git a/src/chains/ethereum/ethereum/tests/provider.test.ts b/src/chains/ethereum/ethereum/tests/provider.test.ts index 27e6117517..6c1c224125 100644 --- a/src/chains/ethereum/ethereum/tests/provider.test.ts +++ b/src/chains/ethereum/ethereum/tests/provider.test.ts @@ -423,16 +423,6 @@ describe("provider", () => { } ); }); - - it("stops responding to RPC methods once disconnected", async () => { - const provider = await getProvider(); - await provider.disconnect(); - - await assert.rejects( - provider.send("eth_getBlockByNumber", ["latest"]), - new Error("Cannot process request, Ganache is disconnected.") - ); - }); }); describe("web3 compatibility", () => { @@ -471,4 +461,104 @@ describe("provider", () => { assert.notStrictEqual(hash, ""); }); }); + + describe("disconnect()", () => { + let provider: EthereumProvider; + + [true, false].forEach(asyncRequestProcessing => { + describe(`asyncRequestProcessing: ${asyncRequestProcessing}`, () => { + beforeEach("Instantiate provider", async () => { + provider = await getProvider({ + chain: { asyncRequestProcessing } + }); + }); + + it("stops responding to RPC methods once disconnected", async () => { + await provider.disconnect(); + + await assert.rejects( + provider.send("eth_getBlockByNumber", ["latest"]), + new Error("Cannot process request, Ganache is disconnected.") + ); + }); + + it("raises the 'disconnect' event", async () => { + const whenDisconnected = provider.once("disconnect"); + await provider.disconnect(); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + + it("successfully processes requests executed before disconnect is called", async () => { + const whenBlockByNumber = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); + const whenDisconnected = provider.disconnect(); + + await assert.doesNotReject( + whenBlockByNumber, + "A call to .request() on the provider before disconnect is called should succeed" + ); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + + it("rejects requests after disconnect is called", async () => { + const whenDisconnected = provider.disconnect(); + const whenBlockByNumber = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); + + await assert.rejects( + whenBlockByNumber, + new Error("Cannot process request, Ganache is disconnected.") + ); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + }); + }); + + describe("without asyncRequestProcessing", () => { + beforeEach("Instantiate provider", async () => { + provider = await getProvider({ + chain: { asyncRequestProcessing: false } + }); + }); + + // we only test this with asyncRequestProcessing: false, because it's impossible to force requests + // to be "pending" when asyncRequestProcessing: true + it("successfully processes started requests, but reject pending requests", async () => { + const active = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); + const pending = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); + + const whenDisconnected = provider.disconnect(); + + await assert.rejects( + pending, + new Error("Cannot process request, Ganache is disconnected."), + "pending tasks should reject" + ); + await assert.doesNotReject(active, "active tasks should not reject"); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + }); + }); }); diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index 73c055fbbd..26ffad62d6 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -83,46 +83,48 @@ export class RequestCoordinator { .catch(noop) .finally(() => { this.runningTasks--; - - if (this.runningTasks === 0 && this.pending.length === 0) { - this.workComplete(); - } else { - this.#process(); + if (this.runningTasks === 0) { + this.#executingTasksFinished(); } + this.#process(); }); } }; - #whenFinished: () => void; - private workComplete() { - if (this.#whenFinished) { - this.#whenFinished(); - this.#whenFinished = undefined; + // resolver which will be set by stop() function if it needs to wait on currently executing tasks to complete + #resolveStopAwaiter: () => void = undefined; + #executingTasksFinished = () => { + if (this.#resolveStopAwaiter !== undefined) { + this.#resolveStopAwaiter(); + this.#resolveStopAwaiter = undefined; } - } + }; /** - * Stop processing tasks - calls to queue(), and resume() will reject with an error indicating that Ganache is + * Stop processing tasks - calls to queue(), and resume() will reject or throw with an error indicating that Ganache is * disconnected. This is an irreversible action. If you wish to be able to resume processing, use pause() instead. * * Note: This will _not_ reject any pending tasks - see rejectPendingTasks() - * @returns Promise - indicating that all currently executing tasks are complete + * @returns Promise - resolves once all currently executing tasks are complete */ - public async stop(): Promise { + public stop = async () => { + if (this.#stopped) { + throw new Error("Already stopped."); + } + this.pause(); this.#stopped = true; - - if (this.runningTasks > 0) { - await new Promise((resolve, _) => { - this.#whenFinished = resolve; - }); + if (this.runningTasks > 0 && this.#resolveStopAwaiter === undefined) { + await new Promise( + (resolve, _) => (this.#resolveStopAwaiter = resolve) + ); } } /** * Reject any pending tasks with an error indicating that Ganache is disconnected. Tasks are rejected in FIFO order. */ - public rejectPendingTasks() { + public rejectPendingTasks = () => { let current: RejectableTask; while ((current = this.pending.shift())) { current.reject( diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index cb0c1ca8f0..d093414beb 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -35,7 +35,7 @@ describe("request-coordinator", () => { }); it("should stop when tasks are queued", async () => { - const uncompletable = coordinator.queue(() => new Promise(noop), this, []); + coordinator.queue(() => new Promise(noop), this, []); await coordinator.stop(); assert(coordinator.paused); @@ -79,6 +79,11 @@ describe("request-coordinator", () => { await assert.doesNotReject(stopped); }); }); + + it("should reject if called a second time", async () => { + coordinator.stop(); + await assert.rejects(coordinator.stop(), new Error("Already stopped.")); + }); }); describe("rejectAllPendingRequests()", () => { From 51781af0fcb24cde48227c9a9c1438b317451a35 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Fri, 5 Aug 2022 13:32:38 +1200 Subject: [PATCH 08/14] Revert a59b00d933d091632335016b87f7e0f768279ddf and 01d62f83f8058dae0aae269ada023b2f8713ad17 --- src/chains/ethereum/ethereum/src/provider.ts | 27 ++--- .../ethereum/ethereum/tests/provider.test.ts | 110 ++---------------- src/packages/utils/src/utils/executor.ts | 4 +- .../utils/src/utils/request-coordinator.ts | 58 +++------ .../utils/tests/request-coordinator.test.ts | 87 +++----------- 5 files changed, 57 insertions(+), 229 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index 5b645ef61f..c93497159b 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -138,9 +138,9 @@ export class EthereumProvider }> implements Provider { - readonly #options: EthereumInternalOptions; - readonly #api: EthereumApi; - readonly #wallet: Wallet; + #options: EthereumInternalOptions; + #api: EthereumApi; + #wallet: Wallet; readonly #executor: Executor; readonly #blockchain: Blockchain; @@ -419,22 +419,19 @@ export class EthereumProvider }; /** - * Disconnect the provider, the underlying blockchain will be stopped. Any tasks currently executing will be completed, - * any pending tasks will be rejected. The returned Promise will resolve when the provider is disconnected, and a - * "disconnect" event will be emitted. - * @returns Promise - resolves when the provider has disconnected + * Disconnect the provider instance. This will cause the underlying blockchain to be stopped, and any pending + * tasks to be rejected. Await the returned Promise to ensure that everything has been cleanly shut down + * before terminating the process. + * @return Promise - indicating that the provider has been cleanly disconnected */ public disconnect = async () => { const executor = this.#executor; - - // we await executor.stop() here to ensure that any currently executing tasks are complete before pulling the - // rug out by stopping the blockchain. - await executor.stop(); - - // we call rejectPendingTasks() _after_ executor.stop() has resolved, to ensure that all tasks are executed in - // FIFO order - executor.rejectPendingTasks(); + // We make a best effort to resolve any currently executing tasks, before rejecting pending tasks. This relies on + // this.#blockchain.stop() waiting to resolve until after all executing tasks have settled. Executor does not + // guarantee that no tasks are currently executing, before rejecting any remaining pending tasks. + executor.stop(); await this.#blockchain.stop(); + executor.rejectPendingTasks(); this.emit("disconnect"); }; diff --git a/src/chains/ethereum/ethereum/tests/provider.test.ts b/src/chains/ethereum/ethereum/tests/provider.test.ts index 6c1c224125..27e6117517 100644 --- a/src/chains/ethereum/ethereum/tests/provider.test.ts +++ b/src/chains/ethereum/ethereum/tests/provider.test.ts @@ -423,6 +423,16 @@ describe("provider", () => { } ); }); + + it("stops responding to RPC methods once disconnected", async () => { + const provider = await getProvider(); + await provider.disconnect(); + + await assert.rejects( + provider.send("eth_getBlockByNumber", ["latest"]), + new Error("Cannot process request, Ganache is disconnected.") + ); + }); }); describe("web3 compatibility", () => { @@ -461,104 +471,4 @@ describe("provider", () => { assert.notStrictEqual(hash, ""); }); }); - - describe("disconnect()", () => { - let provider: EthereumProvider; - - [true, false].forEach(asyncRequestProcessing => { - describe(`asyncRequestProcessing: ${asyncRequestProcessing}`, () => { - beforeEach("Instantiate provider", async () => { - provider = await getProvider({ - chain: { asyncRequestProcessing } - }); - }); - - it("stops responding to RPC methods once disconnected", async () => { - await provider.disconnect(); - - await assert.rejects( - provider.send("eth_getBlockByNumber", ["latest"]), - new Error("Cannot process request, Ganache is disconnected.") - ); - }); - - it("raises the 'disconnect' event", async () => { - const whenDisconnected = provider.once("disconnect"); - await provider.disconnect(); - await assert.doesNotReject( - whenDisconnected, - 'The provider should raise the "disconnect" event' - ); - }); - - it("successfully processes requests executed before disconnect is called", async () => { - const whenBlockByNumber = provider.request({ - method: "eth_getBlockByNumber", - params: ["latest"] - }); - const whenDisconnected = provider.disconnect(); - - await assert.doesNotReject( - whenBlockByNumber, - "A call to .request() on the provider before disconnect is called should succeed" - ); - await assert.doesNotReject( - whenDisconnected, - 'The provider should raise the "disconnect" event' - ); - }); - - it("rejects requests after disconnect is called", async () => { - const whenDisconnected = provider.disconnect(); - const whenBlockByNumber = provider.request({ - method: "eth_getBlockByNumber", - params: ["latest"] - }); - - await assert.rejects( - whenBlockByNumber, - new Error("Cannot process request, Ganache is disconnected.") - ); - await assert.doesNotReject( - whenDisconnected, - 'The provider should raise the "disconnect" event' - ); - }); - }); - }); - - describe("without asyncRequestProcessing", () => { - beforeEach("Instantiate provider", async () => { - provider = await getProvider({ - chain: { asyncRequestProcessing: false } - }); - }); - - // we only test this with asyncRequestProcessing: false, because it's impossible to force requests - // to be "pending" when asyncRequestProcessing: true - it("successfully processes started requests, but reject pending requests", async () => { - const active = provider.request({ - method: "eth_getBlockByNumber", - params: ["latest"] - }); - const pending = provider.request({ - method: "eth_getBlockByNumber", - params: ["latest"] - }); - - const whenDisconnected = provider.disconnect(); - - await assert.rejects( - pending, - new Error("Cannot process request, Ganache is disconnected."), - "pending tasks should reject" - ); - await assert.doesNotReject(active, "active tasks should not reject"); - await assert.doesNotReject( - whenDisconnected, - 'The provider should raise the "disconnect" event' - ); - }); - }); - }); }); diff --git a/src/packages/utils/src/utils/executor.ts b/src/packages/utils/src/utils/executor.ts index 520325c1f1..76923a0fe1 100644 --- a/src/packages/utils/src/utils/executor.ts +++ b/src/packages/utils/src/utils/executor.ts @@ -12,8 +12,8 @@ export class Executor { this.#requestCoordinator = requestCoordinator; } - public stop(): Promise { - return this.#requestCoordinator.stop(); + public stop() { + this.#requestCoordinator.stop(); } public rejectPendingTasks() { diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index 26ffad62d6..f7fb58afce 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -24,10 +24,7 @@ export class RequestCoordinator { * The number of tasks currently being processed. */ public runningTasks: number = 0; - - #paused = true; - #stopped = false; - + #paused: boolean = true; public get paused(): boolean { return this.#paused; } @@ -53,12 +50,6 @@ export class RequestCoordinator { * Resume processing. */ public resume = () => { - if (this.#stopped) { - throw new Error( - "Cannot resume processing requests, Ganache is disconnected." - ); - } - this.#paused = false; this.#process(); }; @@ -83,50 +74,40 @@ export class RequestCoordinator { .catch(noop) .finally(() => { this.runningTasks--; - if (this.runningTasks === 0) { - this.#executingTasksFinished(); - } this.#process(); }); } }; - // resolver which will be set by stop() function if it needs to wait on currently executing tasks to complete - #resolveStopAwaiter: () => void = undefined; - #executingTasksFinished = () => { - if (this.#resolveStopAwaiter !== undefined) { - this.#resolveStopAwaiter(); - this.#resolveStopAwaiter = undefined; - } - }; - /** - * Stop processing tasks - calls to queue(), and resume() will reject or throw with an error indicating that Ganache is + * Stop processing tasks - calls to queue(), and resume() will reject with an error indicating that Ganache is * disconnected. This is an irreversible action. If you wish to be able to resume processing, use pause() instead. * * Note: This will _not_ reject any pending tasks - see rejectPendingTasks() - * @returns Promise - resolves once all currently executing tasks are complete */ - public stop = async () => { - if (this.#stopped) { - throw new Error("Already stopped."); - } - + public stop() { this.pause(); - this.#stopped = true; - if (this.runningTasks > 0 && this.#resolveStopAwaiter === undefined) { - await new Promise( - (resolve, _) => (this.#resolveStopAwaiter = resolve) + + // ensure nothing can be requeued (although tasks can be added directly to this.pending but they will never be + // processed). We make this async to force a Promise return type. + this.queue = async () => { + throw new Error("Cannot process request, Ganache is disconnected."); + }; + + // ensure that processing cannot be resumed. + this.resume = () => { + throw new Error( + "Cannot resume processing requests, Ganache is disconnected." ); - } + }; } /** * Reject any pending tasks with an error indicating that Ganache is disconnected. Tasks are rejected in FIFO order. */ - public rejectPendingTasks = () => { + public rejectPendingTasks() { let current: RejectableTask; - while ((current = this.pending.shift())) { + while(current = this.pending.shift()) { current.reject( new Error("Cannot process request, Ganache is disconnected.") ); @@ -141,11 +122,6 @@ export class RequestCoordinator { thisArgument: any, argumentsList: OverloadedParameters ) => { - if (this.#stopped) { - return Promise.reject( - new Error("Cannot process request, Ganache is disconnected.") - ); - } return new Promise<{ value: ReturnType }>((resolve, reject) => { // const executor is `async` to force the return value into a Promise. const execute = async () => { diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index d093414beb..a1b92e4673 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -33,57 +33,6 @@ describe("request-coordinator", () => { new Error("Cannot process request, Ganache is disconnected.") ); }); - - it("should stop when tasks are queued", async () => { - coordinator.queue(() => new Promise(noop), this, []); - await coordinator.stop(); - - assert(coordinator.paused); - }); - - describe("should wait for in-flight tasks to complete before resolving", () => { - it("when the task resolves", async () => { - const inProgressTask = coordinator.queue(noop, this, []); - coordinator.resume(); - - let isStopped = false; - const stopped = coordinator.stop().then(() => (isStopped = true)); - - await inProgressTask; - assert( - !isStopped, - "return result of RequestCoordinator.stop() resolved before the pending task completed" - ); - - await assert.doesNotReject(stopped); - }); - - it("when the task rejects", async () => { - const inProgressTask = coordinator.queue( - () => Promise.reject(), - this, - [] - ); - coordinator.resume(); - - let isStopped = false; - const stopped = coordinator.stop().then(() => (isStopped = true)); - - // the promise returned from coordinator.queue will resolve, even though the underlying promise rejects - await inProgressTask; - assert( - !isStopped, - "return result of RequestCoordinator.stop() resolved before the pending task completed" - ); - - await assert.doesNotReject(stopped); - }); - }); - - it("should reject if called a second time", async () => { - coordinator.stop(); - await assert.rejects(coordinator.stop(), new Error("Already stopped.")); - }); }); describe("rejectAllPendingRequests()", () => { @@ -96,34 +45,30 @@ describe("request-coordinator", () => { const task = coordinator.queue(noop, this, []); let nextRejectionIndex = taskIndex; - pendingAssertions.push( - task.catch(_ => { - assert.strictEqual( - i, - nextRejectionIndex, - `Rejected in incorrect order, waiting on task at index ${nextRejectionIndex}, got ${i}.` - ); - }) - ); + pendingAssertions.push(task.catch(_ => { + assert.strictEqual(i, nextRejectionIndex, `Rejected in incorrect order, waiting on task at index ${nextRejectionIndex}, got ${i}.`); + })); taskIndex++; - pendingAssertions.push( - assert.rejects( - task, - new Error("Cannot process request, Ganache is disconnected.") - ) - ); + pendingAssertions.push(assert.rejects(task, new Error("Cannot process request, Ganache is disconnected."))); } coordinator.rejectPendingTasks(); await Promise.all(pendingAssertions); + }); - assert.equal( - coordinator.pending.length, - 0, - "Incorrect pending queue length after calling rejectAllPendingRequests" - ); + it("should clear the pending tasks queue", () => { + coordinator.pause(); + + for (let i = 0; i < 10; i++) { + coordinator.queue(noop, this, []); + } + + assert.equal(coordinator.pending.length, 10, "Incorrect pending queue length before calling rejectAllPendingRequests"); + + coordinator.rejectPendingTasks(); + assert.equal(coordinator.pending.length, 0, "Incorrect pending queue length after calling rejectAllPendingRequests"); }); }); }); From 942baccdebc6c3b57580c32bf272cf11488bda51 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Fri, 5 Aug 2022 13:49:35 +1200 Subject: [PATCH 09/14] Best effort to settle tasks in order, added some tests --- src/chains/ethereum/ethereum/src/provider.ts | 14 +-- .../ethereum/ethereum/tests/provider.test.ts | 111 ++++++++++++++++-- src/packages/utils/src/utils/executor.ts | 5 +- .../utils/src/utils/request-coordinator.ts | 40 +++---- .../utils/tests/request-coordinator.test.ts | 2 + 5 files changed, 133 insertions(+), 39 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index c93497159b..5fcb66b54f 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -420,19 +420,17 @@ export class EthereumProvider /** * Disconnect the provider instance. This will cause the underlying blockchain to be stopped, and any pending - * tasks to be rejected. Await the returned Promise to ensure that everything has been cleanly shut down - * before terminating the process. + * tasks to be rejected. Await the returned Promise to ensure that everything has been cleanly shut down before + * terminating the process. * @return Promise - indicating that the provider has been cleanly disconnected */ public disconnect = async () => { - const executor = this.#executor; - // We make a best effort to resolve any currently executing tasks, before rejecting pending tasks. This relies on - // this.#blockchain.stop() waiting to resolve until after all executing tasks have settled. Executor does not - // guarantee that no tasks are currently executing, before rejecting any remaining pending tasks. - executor.stop(); + // executor.stop() will stop accepting new tasks, but will not wait for inflight tasks. These may reject with + // (unhelpful) internal errors. See https://github.com/trufflesuite/ganache/issues/3499 + this.#executor.stop(); await this.#blockchain.stop(); - executor.rejectPendingTasks(); + this.#executor.rejectPendingTasks(); this.emit("disconnect"); }; diff --git a/src/chains/ethereum/ethereum/tests/provider.test.ts b/src/chains/ethereum/ethereum/tests/provider.test.ts index 27e6117517..39ee233026 100644 --- a/src/chains/ethereum/ethereum/tests/provider.test.ts +++ b/src/chains/ethereum/ethereum/tests/provider.test.ts @@ -423,16 +423,6 @@ describe("provider", () => { } ); }); - - it("stops responding to RPC methods once disconnected", async () => { - const provider = await getProvider(); - await provider.disconnect(); - - await assert.rejects( - provider.send("eth_getBlockByNumber", ["latest"]), - new Error("Cannot process request, Ganache is disconnected.") - ); - }); }); describe("web3 compatibility", () => { @@ -471,4 +461,105 @@ describe("provider", () => { assert.notStrictEqual(hash, ""); }); }); + + describe.only("disconnect()", () => { + let provider: EthereumProvider; + + [true, false].forEach(asyncRequestProcessing => { + describe(`asyncRequestProcessing: ${asyncRequestProcessing}`, () => { + beforeEach("Instantiate provider", async () => { + provider = await getProvider({ + chain: { asyncRequestProcessing } + }); + }); + + it("stops responding to RPC methods once disconnected", async () => { + await provider.disconnect(); + + await assert.rejects( + provider.send("eth_getBlockByNumber", ["latest"]), + new Error("Cannot process request, Ganache is disconnected.") + ); + }); + + it("raises the 'disconnect' event", async () => { + const whenDisconnected = provider.once("disconnect"); + await provider.disconnect(); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + + // todo: Reinstate this test when https://github.com/trufflesuite/ganache/issues/3499 is fixed + it.skip("successfully processes requests executed before disconnect is called", async () => { + const whenBlockByNumber = provider.request({ + method: "eth_getProof", + params: ["0xC7D9E2d5FE0Ff5C43102158C31BbC4aA2fDe10d8", [], "latest"] + }); + const whenDisconnected = provider.disconnect(); + + await assert.doesNotReject( + whenBlockByNumber, + "A call to .request() on the provider before disconnect is called should succeed" + ); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + + it("rejects requests after disconnect is called", async () => { + const whenDisconnected = provider.disconnect(); + const whenBlockByNumber = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); + + await assert.rejects( + whenBlockByNumber, + new Error("Cannot process request, Ganache is disconnected.") + ); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + }); + }); + + describe("without asyncRequestProcessing", () => { + beforeEach("Instantiate provider", async () => { + provider = await getProvider({ + chain: { asyncRequestProcessing: false } + }); + }); + + // we only test this with asyncRequestProcessing: false, because it's impossible to force requests + // to be "pending" when asyncRequestProcessing: true + it("successfully processes started requests, but reject pending requests", async () => { + const active = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); + const pending = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); + + const whenDisconnected = provider.disconnect(); + + await assert.rejects( + pending, + new Error("Cannot process request, Ganache is disconnected."), + "pending tasks should reject" + ); + await assert.doesNotReject(active, "active tasks should not reject"); + await assert.doesNotReject( + whenDisconnected, + 'The provider should raise the "disconnect" event' + ); + }); + }); + }); }); diff --git a/src/packages/utils/src/utils/executor.ts b/src/packages/utils/src/utils/executor.ts index 76923a0fe1..659a2a71f8 100644 --- a/src/packages/utils/src/utils/executor.ts +++ b/src/packages/utils/src/utils/executor.ts @@ -12,6 +12,10 @@ export class Executor { this.#requestCoordinator = requestCoordinator; } + /** + * Stop processing requests. We pass this call through to the requestCoordinator, which means that api + * validation will continue to work after calling stop() in execute(). + */ public stop() { this.#requestCoordinator.stop(); } @@ -20,7 +24,6 @@ export class Executor { this.#requestCoordinator.rejectPendingTasks(); } - /** * Executes the method with the given methodName on the API * @param methodName - The name of the JSON-RPC method to execute. diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index f7fb58afce..7ef6417b2c 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -25,6 +25,7 @@ export class RequestCoordinator { */ public runningTasks: number = 0; #paused: boolean = true; + #stopped: boolean = false; public get paused(): boolean { return this.#paused; } @@ -50,6 +51,11 @@ export class RequestCoordinator { * Resume processing. */ public resume = () => { + if (this.#stopped) { + throw new Error( + "Cannot resume processing requests, Ganache is disconnected." + ); + } this.#paused = false; this.#process(); }; @@ -80,38 +86,28 @@ export class RequestCoordinator { }; /** - * Stop processing tasks - calls to queue(), and resume() will reject with an error indicating that Ganache is - * disconnected. This is an irreversible action. If you wish to be able to resume processing, use pause() instead. - * - * Note: This will _not_ reject any pending tasks - see rejectPendingTasks() + * Stop processing tasks - calls to queue(), and resume() will reject with an error indicating + * that Ganache is disconnected. This is an irreversible action. If you wish to be able to resume + * processing, use pause() instead. */ public stop() { this.pause(); - - // ensure nothing can be requeued (although tasks can be added directly to this.pending but they will never be - // processed). We make this async to force a Promise return type. - this.queue = async () => { - throw new Error("Cannot process request, Ganache is disconnected."); - }; - - // ensure that processing cannot be resumed. - this.resume = () => { - throw new Error( - "Cannot resume processing requests, Ganache is disconnected." - ); - }; + this.#stopped = true; } /** - * Reject any pending tasks with an error indicating that Ganache is disconnected. Tasks are rejected in FIFO order. + * All pending tasks will reject with an error indicating that Ganache is disconnected. */ public rejectPendingTasks() { - let current: RejectableTask; - while(current = this.pending.shift()) { + for(const current of this.pending) { current.reject( new Error("Cannot process request, Ganache is disconnected.") ); } + + if (this.pending.length > 0) { + this.pending.splice(0, this.pending.length); + } } /** @@ -122,6 +118,10 @@ export class RequestCoordinator { thisArgument: any, argumentsList: OverloadedParameters ) => { + if (this.#stopped) { + return Promise.reject(new Error("Cannot process request, Ganache is disconnected.")); + } + return new Promise<{ value: ReturnType }>((resolve, reject) => { // const executor is `async` to force the return value into a Promise. const execute = async () => { diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index a1b92e4673..bcfa85d952 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -56,6 +56,8 @@ describe("request-coordinator", () => { coordinator.rejectPendingTasks(); await Promise.all(pendingAssertions); + + assert.equal(coordinator.pending.length, 0, "Coordinator pending list should be empty"); }); it("should clear the pending tasks queue", () => { From c0bd2f8dacb3da80d0bf853b9ce7d771ee6296a7 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Fri, 5 Aug 2022 14:18:53 +1200 Subject: [PATCH 10/14] Fix test to assert rejection order, tidy up --- src/chains/ethereum/ethereum/src/provider.ts | 7 +-- .../ethereum/ethereum/tests/provider.test.ts | 59 ++++++++----------- src/packages/utils/src/utils/executor.ts | 7 ++- .../utils/src/utils/request-coordinator.ts | 13 ++-- .../utils/tests/request-coordinator.test.ts | 54 ++++++++++++----- 5 files changed, 77 insertions(+), 63 deletions(-) diff --git a/src/chains/ethereum/ethereum/src/provider.ts b/src/chains/ethereum/ethereum/src/provider.ts index 5fcb66b54f..0dad1efe3e 100644 --- a/src/chains/ethereum/ethereum/src/provider.ts +++ b/src/chains/ethereum/ethereum/src/provider.ts @@ -420,9 +420,8 @@ export class EthereumProvider /** * Disconnect the provider instance. This will cause the underlying blockchain to be stopped, and any pending - * tasks to be rejected. Await the returned Promise to ensure that everything has been cleanly shut down before - * terminating the process. - * @return Promise - indicating that the provider has been cleanly disconnected + * tasks to be rejected. Emits a `disconnect` event once successfully disconnected. + * @returns Fullfills with `undefined` once the provider has been disconnected. */ public disconnect = async () => { // executor.stop() will stop accepting new tasks, but will not wait for inflight tasks. These may reject with @@ -430,7 +429,7 @@ export class EthereumProvider this.#executor.stop(); await this.#blockchain.stop(); - this.#executor.rejectPendingTasks(); + this.#executor.end(); this.emit("disconnect"); }; diff --git a/src/chains/ethereum/ethereum/tests/provider.test.ts b/src/chains/ethereum/ethereum/tests/provider.test.ts index 39ee233026..549cf59949 100644 --- a/src/chains/ethereum/ethereum/tests/provider.test.ts +++ b/src/chains/ethereum/ethereum/tests/provider.test.ts @@ -462,7 +462,7 @@ describe("provider", () => { }); }); - describe.only("disconnect()", () => { + describe("disconnect()", () => { let provider: EthereumProvider; [true, false].forEach(asyncRequestProcessing => { @@ -473,26 +473,31 @@ describe("provider", () => { }); }); - it("stops responding to RPC methods once disconnected", async () => { - await provider.disconnect(); + it("rejects requests after disconnect() is called", async () => { + provider.disconnect(); + const whenBlockByNumber = provider.request({ + method: "eth_getBlockByNumber", + params: ["latest"] + }); await assert.rejects( - provider.send("eth_getBlockByNumber", ["latest"]), - new Error("Cannot process request, Ganache is disconnected.") + whenBlockByNumber, + new Error("Cannot process request, Ganache is disconnected."), + "Requests made after disconnect is called should reject" ); }); - it("raises the 'disconnect' event", async () => { + it("emits the 'disconnect' event", async () => { const whenDisconnected = provider.once("disconnect"); await provider.disconnect(); await assert.doesNotReject( whenDisconnected, - 'The provider should raise the "disconnect" event' + 'The provider should emit the "disconnect" event' ); }); // todo: Reinstate this test when https://github.com/trufflesuite/ganache/issues/3499 is fixed - it.skip("successfully processes requests executed before disconnect is called", async () => { + it.skip("processes requests executed before disconnect is called", async () => { const whenBlockByNumber = provider.request({ method: "eth_getProof", params: ["0xC7D9E2d5FE0Ff5C43102158C31BbC4aA2fDe10d8", [], "latest"] @@ -501,46 +506,28 @@ describe("provider", () => { await assert.doesNotReject( whenBlockByNumber, - "A call to .request() on the provider before disconnect is called should succeed" + "Currently executing request should resolve" ); await assert.doesNotReject( whenDisconnected, - 'The provider should raise the "disconnect" event' - ); - }); - - it("rejects requests after disconnect is called", async () => { - const whenDisconnected = provider.disconnect(); - const whenBlockByNumber = provider.request({ - method: "eth_getBlockByNumber", - params: ["latest"] - }); - - await assert.rejects( - whenBlockByNumber, - new Error("Cannot process request, Ganache is disconnected.") - ); - await assert.doesNotReject( - whenDisconnected, - 'The provider should raise the "disconnect" event' + 'The provider should emit the "disconnect" event' ); }); }); }); - describe("without asyncRequestProcessing", () => { - beforeEach("Instantiate provider", async () => { + // todo: Reinstate this test when https://github.com/trufflesuite/ganache/issues/3499 is fixed + describe.skip("without asyncRequestProcessing", () => { + // we only test this with asyncRequestProcessing: false, because it's impossible to force requests + // to be "pending" when asyncRequestProcessing: true + it("processes started requests, but reject pending requests", async () => { provider = await getProvider({ chain: { asyncRequestProcessing: false } }); - }); - // we only test this with asyncRequestProcessing: false, because it's impossible to force requests - // to be "pending" when asyncRequestProcessing: true - it("successfully processes started requests, but reject pending requests", async () => { const active = provider.request({ - method: "eth_getBlockByNumber", - params: ["latest"] + method: "eth_getProof", + params: ["0x4Ae2736a3b914C7597131fd1Ef30F74aC4B20874", [], "latest"] }); const pending = provider.request({ method: "eth_getBlockByNumber", @@ -557,7 +544,7 @@ describe("provider", () => { await assert.doesNotReject(active, "active tasks should not reject"); await assert.doesNotReject( whenDisconnected, - 'The provider should raise the "disconnect" event' + 'The provider should emit the "disconnect" event' ); }); }); diff --git a/src/packages/utils/src/utils/executor.ts b/src/packages/utils/src/utils/executor.ts index 659a2a71f8..484c114db5 100644 --- a/src/packages/utils/src/utils/executor.ts +++ b/src/packages/utils/src/utils/executor.ts @@ -20,8 +20,11 @@ export class Executor { this.#requestCoordinator.stop(); } - public rejectPendingTasks() { - this.#requestCoordinator.rejectPendingTasks(); + /** + * Finalise shutdown of the underlying RequestCoordinator. + */ + public end() { + this.#requestCoordinator.end(); } /** diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index 7ef6417b2c..c89b6edbc3 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -96,10 +96,11 @@ export class RequestCoordinator { } /** - * All pending tasks will reject with an error indicating that Ganache is disconnected. + * Finalise shutdown of the RequestCoordinator. Rejects all pending tasks in order. Should be + * called after all in-flight tasks have resolved in order to maintain overall FIFO order. */ - public rejectPendingTasks() { - for(const current of this.pending) { + public end() { + for (const current of this.pending) { current.reject( new Error("Cannot process request, Ganache is disconnected.") ); @@ -119,11 +120,13 @@ export class RequestCoordinator { argumentsList: OverloadedParameters ) => { if (this.#stopped) { - return Promise.reject(new Error("Cannot process request, Ganache is disconnected.")); + return Promise.reject( + new Error("Cannot process request, Ganache is disconnected.") + ); } return new Promise<{ value: ReturnType }>((resolve, reject) => { - // const executor is `async` to force the return value into a Promise. + // const execute is `async` to force the return value into a Promise. const execute = async () => { try { const value = Reflect.apply( diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index bcfa85d952..7b1e758466 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -35,29 +35,43 @@ describe("request-coordinator", () => { }); }); - describe("rejectAllPendingRequests()", () => { + describe("end()", () => { it("should reject pending requests in the order that they were received", async () => { coordinator.pause(); - let taskIndex = 0; + let nextRejectionIndex = 0; const pendingAssertions: Promise[] = []; - for (let i = 0; i < 10; i++) { - const task = coordinator.queue(noop, this, []); - let nextRejectionIndex = taskIndex; - pendingAssertions.push(task.catch(_ => { - assert.strictEqual(i, nextRejectionIndex, `Rejected in incorrect order, waiting on task at index ${nextRejectionIndex}, got ${i}.`); - })); - - taskIndex++; + for (let taskIndex = 0; taskIndex < 10; taskIndex++) { + const task = coordinator.queue(noop, this, []); - pendingAssertions.push(assert.rejects(task, new Error("Cannot process request, Ganache is disconnected."))); + pendingAssertions.push( + task.catch(_ => { + assert.strictEqual( + taskIndex, + nextRejectionIndex, + `Rejected in incorrect order, waiting on task at index ${nextRejectionIndex}, got ${taskIndex}.` + ); + nextRejectionIndex++; + }) + ); + + pendingAssertions.push( + assert.rejects( + task, + new Error("Cannot process request, Ganache is disconnected.") + ) + ); } - coordinator.rejectPendingTasks(); + coordinator.end(); await Promise.all(pendingAssertions); - assert.equal(coordinator.pending.length, 0, "Coordinator pending list should be empty"); + assert.equal( + coordinator.pending.length, + 0, + "Pending array should be empty" + ); }); it("should clear the pending tasks queue", () => { @@ -67,10 +81,18 @@ describe("request-coordinator", () => { coordinator.queue(noop, this, []); } - assert.equal(coordinator.pending.length, 10, "Incorrect pending queue length before calling rejectAllPendingRequests"); + assert.equal( + coordinator.pending.length, + 10, + "Incorrect pending queue length before calling end()" + ); - coordinator.rejectPendingTasks(); - assert.equal(coordinator.pending.length, 0, "Incorrect pending queue length after calling rejectAllPendingRequests"); + coordinator.end(); + assert.equal( + coordinator.pending.length, + 0, + "Incorrect pending queue length after calling end()" + ); }); }); }); From 392b843a37a204b0a3e829c387253d2fb73b7b56 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Thu, 11 Aug 2022 15:33:43 +1200 Subject: [PATCH 11/14] renames and tidy ups as per PR suggestions --- src/chains/ethereum/ethereum/tests/provider.test.ts | 2 +- src/packages/utils/src/utils/request-coordinator.ts | 12 ++++-------- src/packages/utils/tests/request-coordinator.test.ts | 12 +++++++----- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/chains/ethereum/ethereum/tests/provider.test.ts b/src/chains/ethereum/ethereum/tests/provider.test.ts index 549cf59949..9021b6c816 100644 --- a/src/chains/ethereum/ethereum/tests/provider.test.ts +++ b/src/chains/ethereum/ethereum/tests/provider.test.ts @@ -473,7 +473,7 @@ describe("provider", () => { }); }); - it("rejects requests after disconnect() is called", async () => { + it("immediately and syncronously stops accepting request when `disconnect()` is called", async () => { provider.disconnect(); const whenBlockByNumber = provider.request({ method: "eth_getBlockByNumber", diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index c89b6edbc3..18090b92ef 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -100,14 +100,10 @@ export class RequestCoordinator { * called after all in-flight tasks have resolved in order to maintain overall FIFO order. */ public end() { - for (const current of this.pending) { - current.reject( - new Error("Cannot process request, Ganache is disconnected.") - ); - } - - if (this.pending.length > 0) { - this.pending.splice(0, this.pending.length); + while (this.pending.length > 0) { + this.pending + .shift() + .reject(new Error("Cannot process request, Ganache is disconnected.")); } } diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index 7b1e758466..783dbe7b40 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -2,6 +2,8 @@ import assert from "assert"; import { RequestCoordinator } from "../src/utils/request-coordinator"; describe("request-coordinator", () => { + const thisArg = {}; + const paramsArg: any[] = []; const noop = () => undefined; let coordinator: RequestCoordinator; @@ -10,7 +12,7 @@ describe("request-coordinator", () => { }); describe("stop()", () => { - it("should pause processing", () => { + it("should set `paused` property to `true`", () => { coordinator.stop(); assert(coordinator.paused); @@ -29,7 +31,7 @@ describe("request-coordinator", () => { coordinator.stop(); await assert.rejects( - coordinator.queue(noop, this, []), + coordinator.queue(noop, thisArg, paramsArg), new Error("Cannot process request, Ganache is disconnected.") ); }); @@ -43,10 +45,10 @@ describe("request-coordinator", () => { const pendingAssertions: Promise[] = []; for (let taskIndex = 0; taskIndex < 10; taskIndex++) { - const task = coordinator.queue(noop, this, []); + const task = coordinator.queue(noop, thisArg, paramsArg); pendingAssertions.push( - task.catch(_ => { + task.finally(() => { assert.strictEqual( taskIndex, nextRejectionIndex, @@ -78,7 +80,7 @@ describe("request-coordinator", () => { coordinator.pause(); for (let i = 0; i < 10; i++) { - coordinator.queue(noop, this, []); + coordinator.queue(noop, thisArg, paramsArg); } assert.equal( From 469b310b85f1a220b7e330cf3e8e843ceebf8d3e Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Thu, 11 Aug 2022 15:44:42 +1200 Subject: [PATCH 12/14] Oops - paramsArg type mismatch --- src/packages/utils/tests/request-coordinator.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index 783dbe7b40..891dae3351 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -3,7 +3,7 @@ import { RequestCoordinator } from "../src/utils/request-coordinator"; describe("request-coordinator", () => { const thisArg = {}; - const paramsArg: any[] = []; + const paramsArg: [] = []; const noop = () => undefined; let coordinator: RequestCoordinator; From 3f3b8dc25dc03c952b6ef682dff16aa2b7edafd8 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Thu, 11 Aug 2022 16:14:05 +1200 Subject: [PATCH 13/14] Revert finally change --- src/packages/utils/tests/request-coordinator.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/packages/utils/tests/request-coordinator.test.ts b/src/packages/utils/tests/request-coordinator.test.ts index 891dae3351..30880d2b22 100644 --- a/src/packages/utils/tests/request-coordinator.test.ts +++ b/src/packages/utils/tests/request-coordinator.test.ts @@ -48,7 +48,7 @@ describe("request-coordinator", () => { const task = coordinator.queue(noop, thisArg, paramsArg); pendingAssertions.push( - task.finally(() => { + task.catch(() => { assert.strictEqual( taskIndex, nextRejectionIndex, From 4dafd52bbad437211ab6c4fb603909e1c620ce18 Mon Sep 17 00:00:00 2001 From: Jeff Smale <6363749+jeffsmale90@users.noreply.github.com> Date: Fri, 19 Aug 2022 14:00:51 +1200 Subject: [PATCH 14/14] Revert back to re-referencing queue() and resume() functions. Update ConnectorLoader to _not_ maintain a reference to requestCoordinator.resume(), and test obscure edge case. --- src/packages/core/src/connector-loader.ts | 5 ++- .../core/tests/connector-loader.test.ts | 18 ++++++++++ src/packages/utils/src/utils/executor.ts | 7 +++- .../utils/src/utils/request-coordinator.ts | 33 ++++++++++--------- 4 files changed, 45 insertions(+), 18 deletions(-) create mode 100644 src/packages/core/tests/connector-loader.test.ts diff --git a/src/packages/core/src/connector-loader.ts b/src/packages/core/src/connector-loader.ts index cb25e931e4..25cb8a191a 100644 --- a/src/packages/core/src/connector-loader.ts +++ b/src/packages/core/src/connector-loader.ts @@ -47,9 +47,12 @@ const initialize = ( // provider is ready we unpause.. This lets us accept queue requests before // we've even fully initialized. + // The function referenced by requestcoordinator.resume will be changed when + // requestCoordinator.stop() is called. Ensure that no references to the + // function are held, otherwise internal errors may be surfaced. return { connector, - promise: connectPromise.then(requestCoordinator.resume) + promise: connectPromise.then(() => requestCoordinator.resume()) }; }; diff --git a/src/packages/core/tests/connector-loader.test.ts b/src/packages/core/tests/connector-loader.test.ts new file mode 100644 index 0000000000..e8374c951c --- /dev/null +++ b/src/packages/core/tests/connector-loader.test.ts @@ -0,0 +1,18 @@ +import assert from "assert"; +import loader from "../src/connector-loader"; + +describe("connector-loader", () => { + describe("initialize", () => { + it("the returned promise should reject, if disconnect() is called before the provider is ready", async () => { + const { promise, connector } = loader.initialize({}); + connector.provider.disconnect(); + + // This assertion ensures that the "stopped" queue() method that is + // assigned in request-coordinator.stop() is called correctly. + await assert.rejects( + promise, + new Error("Cannot resume processing requests, Ganache is disconnected.") + ); + }); + }); +}); diff --git a/src/packages/utils/src/utils/executor.ts b/src/packages/utils/src/utils/executor.ts index 484c114db5..561275df37 100644 --- a/src/packages/utils/src/utils/executor.ts +++ b/src/packages/utils/src/utils/executor.ts @@ -13,7 +13,7 @@ export class Executor { } /** - * Stop processing requests. We pass this call through to the requestCoordinator, which means that api + * Stop processing requests. We pass this call through to the requestCoordinator, which means that api * validation will continue to work after calling stop() in execute(). */ public stop() { @@ -58,6 +58,11 @@ export class Executor { // just double check, in case a API breaks the rules and adds non-fns // to their API interface. if (typeof fn === "function") { + // The function referenced by requestcoordinator.queue will be changed + // when requestCoordinator.stop() is called. Ensure that no references + // to the function are held, otherwise internal errors may be + // surfaced. + // queue up this method for actual execution: return this.#requestCoordinator.queue(fn, api, params); } diff --git a/src/packages/utils/src/utils/request-coordinator.ts b/src/packages/utils/src/utils/request-coordinator.ts index 18090b92ef..0c5837dc81 100644 --- a/src/packages/utils/src/utils/request-coordinator.ts +++ b/src/packages/utils/src/utils/request-coordinator.ts @@ -24,8 +24,8 @@ export class RequestCoordinator { * The number of tasks currently being processed. */ public runningTasks: number = 0; + #paused: boolean = true; - #stopped: boolean = false; public get paused(): boolean { return this.#paused; } @@ -51,11 +51,6 @@ export class RequestCoordinator { * Resume processing. */ public resume = () => { - if (this.#stopped) { - throw new Error( - "Cannot resume processing requests, Ganache is disconnected." - ); - } this.#paused = false; this.#process(); }; @@ -86,13 +81,25 @@ export class RequestCoordinator { }; /** - * Stop processing tasks - calls to queue(), and resume() will reject with an error indicating - * that Ganache is disconnected. This is an irreversible action. If you wish to be able to resume - * processing, use pause() instead. + * Stop processing tasks - calls to queue(), and resume() will reject with an + * error indicating that Ganache is disconnected. This is an irreversible + * action. If you wish to be able to resume processing, use pause() instead. + * + * Note: this changes the references of this.resume and this.queue. Any code + * that maintains references to the values referenced by this.resume or + * this.queue, could have unintended consequences after calling this.stop(). */ public stop() { this.pause(); - this.#stopped = true; + this.resume = () => { + throw new Error( + "Cannot resume processing requests, Ganache is disconnected." + ); + }; + + this.queue = async () => { + throw new Error("Cannot process request, Ganache is disconnected."); + }; } /** @@ -115,12 +122,6 @@ export class RequestCoordinator { thisArgument: any, argumentsList: OverloadedParameters ) => { - if (this.#stopped) { - return Promise.reject( - new Error("Cannot process request, Ganache is disconnected.") - ); - } - return new Promise<{ value: ReturnType }>((resolve, reject) => { // const execute is `async` to force the return value into a Promise. const execute = async () => {