fix: add error checks for integrated devnet process #133
Conversation
There was a problem hiding this comment.
Which benefits does this PR introduce and #132 doesn't? I took inspiration from your PR and extracted the server-alive functionality to a helper function. I also added the initial checking not to bother spawning if address already occupied. Other than that, #132 will introduce tests and the implementation looks simpler (perhaps it is missing some corner case).
|
|
||
| // handle unexpected close of process | ||
| this.childProcess.on("close", (code) => { | ||
| const isAbnormalExit = this.childProcess != null; |
There was a problem hiding this comment.
I don't like how to check if devnet is up and running we have to check multiple variables (connected and childProcess).
There was a problem hiding this comment.
Yes, I don't like it either, but that's the only way to correctly separate failure-to-connect from crashed-while-processing-tx scenarios.
If the process was killed by the user or runtime (this.childProcess == null), then this shouldn't throw an error.
| this.childProcess = null; | ||
| if (code !== 0 && isAbnormalExit) { | ||
| if (this.connected) { | ||
| throw new Error(`integrated-devnet exited with code=${code} while processing transactions`); |
There was a problem hiding this comment.
Inside the plugin we try to throw instances of HardhatPluginError.
| const oneSleepMillis = 100; | ||
|
|
||
| // keep checking until process has failed/exited | ||
| while (this.childProcess) { |
There was a problem hiding this comment.
Any particular reason why you replaced a for-loop with a while-loop?
There was a problem hiding this comment.
Yes, the for-loop didn't account for delays caused by the internal event loop, and web requests. Once it was converted to a timer based loop, it made sense to have a while loop
| reject(error); | ||
| }); | ||
|
|
||
| // capture the most recent message from stderr | ||
| this.childProcess.stderr.on("data", (data: any) => { |
There was a problem hiding this comment.
How does this work? Are data chunks guaranteed to be meaningful standalone messages or do they need to be collected and merged?
There was a problem hiding this comment.
This is up to how the internal application flushes its stderr. It's out of our control. For most use cases this will work as expected, eg: print('something went wrong!', file=sys.stderr, flush=True) will give something went wrong!\n
Typically most of the information is in the latest flushed error, however for extremely long errors, this would need a dedicated log file...
| if (await this.isServerAlive()) { | ||
| throw new Error(`integrated-devnet cannot start because existing devnet already running on ${this.host}:${this.port}`) | ||
| } | ||
|
|
||
| this.childProcess = await this.spawnChildProcess(); | ||
|
|
||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Do all the child process event handlers need to be configured inside the Promise?
There was a problem hiding this comment.
Only the stderr event handler can be extracted since it doesn't depend on reject()
|
Thanks for the clarification, I merged your PR into a non-master branch, and will combine it with #132 |
Usage related changes
Development related changes
Checklist:
testdirectorypluginbranch ofstarknet-hardhat-example:test.shto use my example repo branchtest.shto to use the original branch (after the example repo PR has been merged)