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

WIP: builder interface for constructing servers, rework server startup code paths #1120

Closed
wants to merge 4 commits into from

Conversation

davepacheco
Copy link
Collaborator

Particularly after #1115, the arguments for the HttpServerStarter::new* family of functions is getting unwieldy. I wanted to take a stab at a builder-based constructor.

But it seemed excessive to create a Builder that creates a Starter that creates the Server. So I wanted to see if I could obviate the Starter altogether. I looked into why it exists and I found #81, which I think made sense at the time. But at this point, HttpServerStarter has only one thing you can do with it, which is to call start(), and that consumes self. By the time you have your Starter, it has already bound to the listening socket and there are no new ways that it can fail to start. What start does is spawn tokio tasks to actually accept connections and handle requests. But I can't see why it's useful to separate these steps. So in this implementation, you call ServerBuilder::new(), customize it, then call build() (which consumes self and can fail), and then you have your started HttpServer.

In the process of doing this, I found that the existing code had quite a lot more bifurcation than seemed necessary in both types and code paths for TLS vs. non-TLS. So I wound up unifying these paths quite a bit. I think there's probably more cleanup that could be done here.

Since I was revisiting this interface, I also defined a new BuildError enum using thiserror. We'd previously been using a GenericError which was just a typedef for a boxed std error (with a TODO to come up with something better). I changed all the startup code paths that produce errors to produce BuildError instead.

I believe this is not a breaking change because I implemented the existing HttpServerStarter in terms of the new builder.


In summary: I started off trying to do one thing but wound up reworking basically the whole server startup and connection handling paths for both TLS and non-TLS. That's smaller than it sounds, but assuming we like this (which I think is not a given), this could use some careful review.

Here are some other next steps I'm considering:

  • Document the new builder and related structs and change examples and the documentation to use it.
  • I'm thinking that we could preserve the separate "start" step in case we find that consumers still need it, while still making a simpler path for people who don't. We could do this by saying that ServerStarter::build_starter() returns an HttpServerStarter that looks just like the one we have today (i.e., one method called start()). Then we'd create ServerStarter::start() that just calls build_starter()?.start().
  • I could split up the two main parts of this PR.
    • first PR would add the new builder as described above, but not rewrite the startup path to unify the TLS and non-TLS paths.
    • second PR would unify the TLS and non-TLS paths

@davepacheco
Copy link
Collaborator Author

Closing in favor of #1121 and #1122.

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.

1 participant