[http server]: use tokio::spawn internally in HttpServer::start and return StopHandle
#402
Conversation
niklasad1
changed the title
HttpServer::start and return StopHandle
| /// Blocks indefinitely until the server is stopped. | ||
| pub async fn wait_for_stop(&self) { | ||
| self.stop_handle.lock().await; | ||
| pub fn stop(mut self) -> Result<tokio::task::JoinHandle<()>, Error> { |
There was a problem hiding this comment.
//cc @maciejhirsz I tried to unify the APIs but the underlying impl is different and fine by me at least.
I guess it would be nice to replace the stop_sender with your AtomicUsize waker because https://docs.rs/hyper/0.14.13/hyper/server/struct.Server.html#method.with_graceful_shutdown requires at future to be resolved but didn't work out of the box.....
any suggestions?
There was a problem hiding this comment.
I think it's fine for now, I need to debug the ws one first, might have some suggestions for it afterwards.
There was a problem hiding this comment.
I think this is fine too; this flavour is a bit easier to read imo.
| /// Configure a custom [`tokio::runtime::Handle`] to run the server on. | ||
| /// | ||
| /// Default: [`tokio::spawn`] | ||
| pub fn custom_tokio_runtime(mut self, rt: tokio::runtime::Handle) { |
There was a problem hiding this comment.
we might want to re-export Handle but I decided not because the library is tightly-coupled to tokio and if the wrong tokio version is used a compile error is more likely than a runtime error.
There was a problem hiding this comment.
Nice! I was thinking about adding something like this for ws for testing, but that ended ultimately not being necessary so I didn't bother. Should we add it there too?
There was a problem hiding this comment.
I have already added it to the WS server if that's ok? :)
This reverts commit 0432f6b.
| /// Configure a custom [`tokio::runtime::Handle`] to run the server on. | ||
| /// | ||
| /// Default: [`tokio::spawn`] | ||
| pub fn custom_tokio_runtime(mut self, rt: tokio::runtime::Handle) { |
There was a problem hiding this comment.
Nice! I was thinking about adding something like this for ws for testing, but that ended ultimately not being necessary so I didn't bother. Should we add it there too?
Co-authored-by: Maciej Hirsz <1096222+maciejhirsz@users.noreply.github.com>
| /// | ||
| /// Default: [`tokio::spawn`] | ||
| pub fn custom_tokio_runtime(mut self, rt: tokio::runtime::Handle) { | ||
| self.settings.tokio_runtime = Some(rt); |
There was a problem hiding this comment.
I added this to the WS Server too...
| /// Blocks indefinitely until the server is stopped. | ||
| pub async fn wait_for_stop(&self) { | ||
| self.stop_handle.lock().await; | ||
| pub fn stop(mut self) -> Result<tokio::task::JoinHandle<()>, Error> { |
There was a problem hiding this comment.
I think this is fine too; this flavour is a bit easier to read imo.
Changes the API to return the
StopHandleafterHttpServer::startinstead of having to call a separate methodhandle....