feat: shutdown in reverse dependency order #147
Conversation
There was a problem hiding this comment.
Hey @anmonteiro,
Thanks for the PR, there are definitely scenarios where this feature is required.
I am a bit concerned that for cases where this is not required, this will make the shutdown sequence much longer.
Since it's a behavior change I don't think we can make it the new default.
A global is_ordered_shutdown flag (default false) will do the trick.
Will you be able to add a few ShutDownProject() tests to system_test.go to make sure that it works as expected?
P.S.
Loved the algorithm you used to wait for the dependencies' completion without shutting them down. Nice!
|
@F1bonacc1 thanks for the early draft review. I believed I addressed all your review comments in the latest commit. Just missing the tests, which I'm working on right now. |
|
added a skeleton of a test case shutting down the runner and checking for 0 running processes, but I'm not sure how to check that the processes are actually stopped in the order we expect. |
| isDaemon: true | ||
| shutdown: | ||
| command: | | ||
| echo "C: exiting" |
There was a problem hiding this comment.
You could write these to a file, then assert at the end for the contents to be ordered backwards.
| echo "C: exiting" | |
| echo "C: exiting" >> shutdown-inorder.log | |
| sleep 1 |
There was a problem hiding this comment.
Nice, thanks. I cherry-picked it to this branch.
|
Not sure why it fails in CI, |
src/app/system_test.go
Outdated
| return | ||
| } | ||
| runningProcesses := 0 | ||
| for _, proc := range runner.runningProcesses { |
There was a problem hiding this comment.
Race condition here.
You can replace this block with runner.GetProcessesState() (plural)
Race condition on |
anmonteiro
force-pushed
the
anmonteiro/shutdown-in-order
branch
from
8941afb to
df2fdd4
Compare
|
I actually believe it might be the log rendering using ANSI escape codes but not the wanted string slice. |
|
pushed a fix using |
|
Looks good! Thanks! |
|
Will you be able to squash? |
address code review fix: unused variable refactor: smaller functions test: shutdown in order Add shutdown order test fix: tests
anmonteiro
force-pushed
the
anmonteiro/shutdown-in-order
branch
from
4057c9d to
dc044a5
Compare
|
Absolutely. Done. |
processes that depend on others to start may also need to finish before them and perform some cleanup. This change makes sure that's the case.
fixes #145