Small cleanup in App and Node
#3962
Conversation
geoff-vball
force-pushed
the
gstuart/small-cleanup
branch
from
d1bffe0 to
b8d9b39
Compare
It seems to me that the |
While that's true, we should note that all exit codes that are of endogenous reasons are 1, and all of exogenous reasons are 0, so in practice - it is highly unlikely that this can have a race. |
|
@yacovm A second invocation of |
|
Oh you're right! My bad :-) |
joshua-kim
requested review from
a team and
samliok
and removed request for
a team
|
|
||
| // Stop notifies the application to exit and returns immediately. | ||
| // Stop should only be called after [Start]. | ||
| // It is safe to call Stop multiple times. | ||
| Stop() error | ||
| Stop() | ||
|
|
||
| // ExitCode should only be called after [Start] returns with no error. It |
There was a problem hiding this comment.
This comment should be updated
| @@ -129,9 +128,7 @@ func Run(app App) int { | |||
| close(stackTraceSignal) | |||
|
|
|||
| // if there was an error closing or running the application, report that error | |||
There was a problem hiding this comment.
Comment should be updated
| @@ -148,7 +145,7 @@ type app struct { | |||
| // Start the business logic of the node (as opposed to config reading, etc). | |||
| // Does not block until the node is done. Errors returned from this method | |||
| } | ||
|
|
||
| // Stop attempts to shutdown the currently running node. This function will | ||
| // return immediately. | ||
| func (a *app) Stop() error { | ||
| // block until shutdown is complete. |
There was a problem hiding this comment.
This comment change isn't really inaccurate (at least based on the intention of the original code).
Calling Shutdown(0) on the node doesn't actually block until shutdown finishes. Shutdown finishes when Dispatch() returns. Notice that Shutdown only starts the shutdown process.
|
|
||
| // If the P2P server isn't running, shut down the node. | ||
| // If node is already shutting down, this does nothing. | ||
| // If node is already shutting down, this does not tigger shutdown again, | ||
| // and blocks until the shutdown is complete. |
There was a problem hiding this comment.
| // and blocks until the shutdown is complete. | |
| // and blocks until the shutdown returns. |
| @@ -682,7 +676,8 @@ func (n *Node) Dispatch() error { | |||
| ) | |||
| } | |||
| // If the API server isn't running, shut down the node. | |||
| // If node is already shutting down, this does nothing. | |||
| // If node is already shutting down, this does not tigger shutdown again, | |||
| // and blocks until the shutdown is complete. | |||
There was a problem hiding this comment.
| // and blocks until the shutdown is complete. | |
| // and blocks until the shutdown returns. |
Why this should be merged
While preparing a new RPC-signer PR, I noticed a few things that could be improved/simplified. There are 4 different atomic commits - feel free to take or leave any of them :)
How this works
shuttingDownatomic flag which could lead toshuttingDownExitCodebeing set more than once.App-Start(),Stop(),ExitCode()always returnnilerrors. I removed the error returns from the function signatures, which makes the possible code paths much easier to reason about.DoneShuttingDowninNodeis redundant, because theOnce.Do()insideShutdown()will block until shutdown is completed.n.Net.Dispatch()is used as the return value, but only ~25 lines later. Renamed toretErrto make it more clear that it is the error that should be returned.How this was tested
Existing tests
Need to be documented in RELEASES.md?
No