-
Notifications
You must be signed in to change notification settings - Fork 678
fix: handle initialization errors on server.listen, handle fallback rejections #1227
Conversation
blockLogs: null | ||
}); | ||
({ stateRoot } = latest.header); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just wrapping things in a try/catch
await this.fallback.initialize(); | ||
await database.initialize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed because previously, if the fallback
throws while database is still starting it becomes very tricky to ensure we can wait for the database to finish before trying to shut the blockchain class down. We can do it, and I did, but it was a lot of code involving handling the weird return values of Promise.allSettled
combined with throwing an AggregateError
in some cases and not in others.
// stop the polling miner, if necessary | ||
clearTimeout(this.#timer); | ||
|
||
// clean up listeners | ||
this.vm.removeAllListeners(); | ||
this.vm && this.vm.removeAllListeners(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now call stop
internally on initialization exceptions we might not have everything ready, so we check for things' existence before trying to clean them up.
@@ -172,7 +172,7 @@ export class HttpHandler extends BaseHandler implements Handler { | |||
if ("result" in result) { | |||
return result.result; | |||
} else if ("error" in result) { | |||
throw result.error; | |||
throw new CodedError(result.error.message, result.error.code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't throw plain objects, kids.
return { | ||
connector, | ||
promise: connectPromise.then(requestCoordinator.resume) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give connector loader callers, like server.ts, the ability to handle start up promise rejections.
// upgrading Typescript. TODO: if Typescript is upgraded to 4.2.3+ | ||
// then this line could be removed and `Promise.allSettled` below | ||
// could replaced with `allSettled`. | ||
allSettled.shim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved this block to the global scope.
// Since the ConnectorLoader starts an async promise that we intentionally | ||
// don't await yet we keep the promise around for something else to handle | ||
// later. | ||
this.#initializer = Promise.all([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this promise is awaited in the listen
function below, and that is where a Connector/Provider's initialization rejections will get handled now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one clarification comment to your annotations.
fixes #938
This is a all weird because I want users to be able to do const provider = Ganache.provider()... notice there is no await here! 👀 Ganache.provider does approx 3,452,345 async things on start up though, and if any of these promises reject no one is there to listen to that rejection; so node complains (but doesn't crash in Node v10-14).
Unfortunately, this PR does not fix this problem for Ganache.provider(), but does fix it for Ganache.server() by having the server.listen function, which is async, handle promise rejections "eventually".
In order to get the provider to handle these errors we'd have to hook into the first async method the user calls after we have finished (failing) initialization... which would probably be something like await provider.request({method:"eth_accounts", params: []})... which is a weird place to throw an initialization error 😕.