Inventing the service trait, example doesn't work?

[saward]

Hi,

I'm in the process of learning async for Rust, and looking at Tokio tower+axum at the same time, to understand how all the pieces fit together. Reading '2021-05-14-inventing-the-service-trait', early on it gives this example:

If we want our server to be able to handle a large number of concurrent connections, we need to be able to serve other requests while we wait for that request to complete asynchronously. Let's fix this by having the handler function return a future:

impl Server {
    async fn run<F, Fut>(self, handler: F) -> Result<(), Error>
    where
        // `handler` now returns a generic type `Fut`...
        F: Fn(HttpRequest) -> Fut,
        // ...which is a `Future` whose `Output` is an `HttpResponse`
        Fut: Future<Output = HttpResponse>,
    {
        let listener = TcpListener::bind(self.addr).await?;

        loop {
            let mut connection = listener.accept().await?;
            let request = read_http_request(&mut connection).await?;

            // Await the future returned by `handler`
            let response = handler(request).await;

            write_http_response(connection, response).await?;
        }
    }
}

The preceding text makes it sound like these changes will allow the server to handle a large number of concurrent requests, but it doesn't, does it? New connections come in inside the loop, but if it's handling another request, then it's awaiting the handler for that request and so won't read any new requests until the current one is completed.

This might be nit-picking, but as mentioned, I'm in the process of learning Rust async and wasn't sure when I read this if there's something else going on here that I don't understand. To help understand and practice I implemented a working example (using block_on in the main function), and it acted as I expected, handling requests only one after the other.

[davidpdrsn]

So technically you're right, however this is not meant to be a general introduction to async or how to write HTTP servers. Its intended to show how things might fit together and to motivate the Service trait.

It would be more correct to add tokio::spawn such that requests are handled concurrently but that would also make the example more complicated and since its just example code so I don't think thats worth it.

What do you think?

[saward]

I absolutely understand the desire to keep this brief. As mentioned, I'm only learning these things, and I know Tokio developers know what they're doing, so I wasn't sure if it was (a) my understanding that was wrong, or (b) the example is being deliberately brief and incomplete because it isn't relevant to the main point. I might be the only one who read this and was unsure which of (a) or (b) applied :)

If you are interested in a "fix" for people like myself, I think a very short note would be sufficient. Something that communicates that more needs to be done here to make it truly concurrent, but the examples are being kept brief for the sake of the main point.

Just as an example, replace:

Let's fix this by having the handler function return a future

With

A first step towards this is to make the handler function return a future

[saward]

Happy for you to close this issue and do nothing too. Or I can submit a PR if you like.

[Darksonn]

I think that adding a tokio::spawn would improve the example.

[davidpdrsn]

@saward do you wanna make a PR that adds a spawn?

[saward]

Would it be something as simple as the following, for each of the relevant locations in the article?

impl Server {
    async fn run<F, Fut>(self, handler: F) -> Result<(), Error>
    where
        // `handler` now returns a generic type `Fut`...
        F: Fn(HttpRequest) -> Fut,
        // ...which is a `Future` whose `Output` is an `HttpResponse`
        Fut: Future<Output = HttpResponse>,
    {
        let listener = TcpListener::bind(self.addr).await?;

        loop {
            let mut connection = listener.accept().await?;
            let request = read_http_request(&mut connection).await?;

            task::spawn(async move {
                // Await the future returned by `handler`
                let response = handler(request).await;

                write_http_response(connection, response).await?;
            });
        }
    }
}

[davidpdrsn]

That looks good to me!

[saward]

Created the PR, let me know if I need to make any changes. Thanks!

[saward]

Sorry, just looking at the doc now, I'm not sure I should have added the task::spawn to the first example. If you agree, I'll create a new PR removing it from that first one.

[davidpdrsn]

The spawn is fine but we shouldn't .await the handler since its not async. I've made a PR for that https://github.com/tokio-rs/website/pull/660