Skip to content

Make client lifetime and shutdown more explicit in client example #1568

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

Closed

Conversation

dekellum
Copy link

This is complimentary/in-addition to #1565. Originally I figured we might be able to resolve that one first, but then I found I needed to also reference this proposal.

Summary of examples/client.rs changes:

  • Uses spawn, which is more async™ than run, and thus more educational for a real application.
  • Avoids the magic of lazy(Fn) and moving the Client instance into a single request future. Thus this example is more amendable to multiple-requests on a shared Client for keep-alive, etc.
  • Makes the shutdown sequence explicit and clarifies potential pitfalls.
  • Adds a working default URL as a convenience, not unlike the doctest. HTTP-only websites are getting harder to find!

Note (as in #1565) tokio::runtime::Runtime isn't currently re-exported in hyper::rt. I could add it there instead, if desired (though I wonder about the sustainability of this re-export approach.)

The client sections of the guide also uses this example, so with some positive indication, I could pursue a compatible guide change PR.

@seanmonstar
Copy link
Member

I think there is value in showing in the examples that you can just use run in many cases. Since the concern is around forgetting to put a Client in a lazy future and thus keeping the runtime from shutting down, what if there was just a comment added at that line about that?

@seanmonstar
Copy link
Member

Thanks for the efforts to improve the documentation, I greatly appreciate it! I'm going to close this for now, and allow discussion to happen in #1571. If we find ways in that issue to improve things, new PRs can always be submitted.

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