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

Guarantee return value of serve #2895

Closed
wants to merge 1 commit into from

Conversation

SabrinaJewson
Copy link
Contributor

Motivation

Currently, serve and with_graceful_shutdown do not document anything about their return values. The type system says that both return io::Result<()>, but in context it is not obvious how this should be interpreted, and the user is left to guess.

As it happens, in the current implementation it is impossible for serve to ever return, and likewise with_graceful_shutdown will never error. It seems unlikely to me that we would want to change this behaviour, and so it would be useful if people could instead rely on it.

Alternatively, if we do plan on allowing serve to return or error in future, this fact should be documented, and it should be specified what action the user is expected to take in that scenario (quit the app? restart the server?).

Solution

This commit adds to the documentation a guarantee that Serve and WithGracefulShutdown are both infallible, that Serve never completes successfully, and that WithGracefulShutdown only completes after its signal future has finished.

It refactors the implementations of both to guarantee this fact in the type system as well.

Future Possibilities

Of course, it would be nice if serve would instead return ! and with_graceful_shutdown (), but that will have to wait for a breaking change.

This commit adds to the documentation a guarantee that `Serve` and
`WithGracefulShutdown` are both infallible, that `Serve` never completes
successfully, and that `WithGracefulShutdown` only completes after its
signal future has finished.

It refactors the implementations of both to guarantee this fact in the
type system as well.
@jplatte
Copy link
Member

jplatte commented Sep 27, 2024

How much do you care about this being in v0.7.x? If you don't care so much, I'd like to (only) revisit this after #2479 (which I want to rebase soon) and actually implement the breaking change you mention in future possibilities if still possible and correct.

@SabrinaJewson
Copy link
Contributor Author

Oh, I see! That would be very nice :)

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.

2 participants