main: Slightly rework main hub and gui lifecycle. #356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davecgh
force-pushed
the
main_rework_lifecycle
branch
2 times, most recently
from
9e8d19e
to
e91ad5f
Compare
jholdstock
reviewed
Sep 12, 2023
| return nil | ||
| } | ||
| if err := runHub(ctx, hub); err != nil { | ||
| // Ensure the GUI is signaled to shutdown. |
There was a problem hiding this comment.
Despite calling cancel here, the gui has no waitgroup (or any other mechanism of signalling that it has completed shutdown) so there is no guarantee that it will shutdown completely before the process terminates. Thats an existing problem though, and we can fix that in a later PR.
There was a problem hiding this comment.
Right. My hope is that separating it out like this helps show the problem with the current design where it passes the cancel func down deep into the innards of something that does not control the overall process and thus shouldn't be able to just pull the rug out from everything as it's doing now.
jholdstock
approved these changes
Sep 12, 2023
This moves the creation of the context into the main code path, splits the creation logic for the hub and gui into independent funcs, and arranges for the main code path to create those instances and run them. Since the pool struct is no longer necessary, both it and the associated newPool func are removed. This approach is preferred because it provides the main code path with full control over the lifecycle of the context instead of stuffing it into a separate struct which makes it much harder to reason about and has ultimately led to the use of the cancel func deep in the innards of the hub which should not have that level of control since it is not the coordinator and thus does not have enough details to know when it's actually safe or even should be forcing a shutdown. This commit does not address the latter part since it will require more in-depth changes to decouple it properly. However, this is a good start at reducing the overly-tight coupling of the lifecycle that exists as a result.
davecgh
force-pushed
the
main_rework_lifecycle
branch
from
e91ad5f
to
2e8153a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is rebased on #355.
This moves the creation of the context into the main code path, splits the creation logic for the hub and gui into independent funcs, and arranges for the main code path to create those instances and run them.
Since the pool struct is no longer necessary, both it and the associated
newPoolfunc are removed.This approach is preferred because it provides the main code path with full control over the lifecycle of the context instead of stuffing it into a separate struct which makes it much harder to reason about and has ultimately led to the use of the cancel func deep in the innards of the hub which should not have that level of control since it is not the coordinator and thus does not have enough details to know when it's actually safe or even should be forcing a shutdown.
This commit does not address the latter part since it will require more in-depth changes to decouple it properly. However, this is a good start at reducing the overly-tight coupling of the lifecycle that exists as a result.