Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webapi: Rework create and run lifecycles. #434

Merged
merged 5 commits into from Sep 16, 2023

Conversation

jholdstock
Copy link
Member

Huge diff here but a lot of it is just renaming. Commits 1, 3 and 4 contain the important changes.

It's generally good practice to first create everything that can fail
and then create the final instance at once with the results versus doing
it piecemeal.

Piecemeal creation is typically more error prone and, while not a huge
concern here, it also ends up needlessly creating objects that are just
thrown away in the event of a later error.
Exporting this struct is a step towards breaking up the Start func into
a New func and Run func, enabling calling code to use:

    api := webapi.New()
    api.Run()

WebAPI is a more suitable name than server because it matches the
package name, and also it helps to distinguish WebAPI from the HTTP
server it uses internally (ie. webapi.server rather than server.server).
Separating the single func grants the calling code greater control over
the webapi lifecycle.
This updates the Run func to make it blocking. When the provided context
is canceled the server is cleanly shut down and the func returns.
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like nice changes overall.

internal/webapi/webapi.go Outdated Show resolved Hide resolved
The server will shutdown cleanly even if the passed context is already
canceled, so there is no need to pass a new context and blindly guess at
how long the server may need to stop.
@jholdstock jholdstock merged commit 61b6460 into decred:master Sep 16, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants