Refactor server spawning to be synchronous and infallible #2097
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
Perform all IO operations in the spawned task.
Perform all IO operations in the spawned task.
Return an error only from the spawned task.
There's no need to delay it.
There aren't any failure scenarios for them.
The fields can be cloned directly in the `async` block, just like what is done with the `server_config.internal_network` value.
There's no need to keep them in the block, now that they are synchronous.
Make it explicit that the order of the futures doesn't matter.
ma2bd
approved these changes
Jun 4, 2024
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.
Motivation
While refactoring to replace
tokio::spawns with spawning onJoinSets, I realized it would help if the spawning of servers was synchronous and infallible. That means that thespawn*methods only spawn tasks, and do no other work.Proposal
Refactor server
spawn*methods to be infallible and (mostly) synchronous. Thespawn_{grpc,simple}methods inlinera-serviceare still asynchronous but they onlyawaitthe collection of the handles, which can be done in a spawned task as well. However, that requires some further refactoring so that the caller awaits all the tasks, which I left for the next PR.In terms of functionality, the visible part of the changes is error reporting. Before it would log a separate error message if an error occurred while a task was starting (for example when parsing an IP address or binding to a socket). With this PR, all errors are logged by the same expression which handles any error during the spawn and during the execution.
Test Plan
This is an internal refactor. No new coverage is needed and CI should catch any regressions.
Release Plan
This is an internal refactor, so nothing is needed.
Links