Use a shared tokio runtime for a blocking rest client

[Tuetuopay]

This avoids creating a whole new runtime for each request being made, which avoids duplicating IO and time drivers (threads being spawned).

As per the tokio docs, a current_thread runtime can be used from multiple concurrent threads, so this is still thread safe.

[spietika]

Very good! I'm travelling this week so I will look at this and the other PR during the weekend.

[spietika]

Overall the PR looks good, but there is one thing I would change.

Now impl From<AsyncRestClient> for RestClient can panic. Even though it is probably very unlikely that building the runtime fails, I would still prefer to propagate the error to user application. Especially since the method where into() is called already returns Result.

I'm not sure if there is a convenient way to propagate error from From trait, but the code could be refactored to avoid that.

I was thinking something like this:

in blocking.rs:

    pub fn from_async(inner_client: AsyncRestClient) -> Result<RestClient, Error> {
        if let Ok(runtime) = Builder::new_current_thread().enable_all().build() {
            Ok(RestClient {
                inner_client,
                runtime,
            })
        } else {
            Err(Error::HttpClientError)
        }
    }

and in lib.rs:

    pub fn blocking(self, url: &str) -> Result<blocking::RestClient, Error> {
        if let Ok(inner) = RestClient::with_builder(url, self) {
            blocking::RestClient::from_async(inner)
        } else {
            Err(Error::HttpClientError)
        }
    }

Your thoughts on this approach?

[Tuetuopay]

We could switch to the TryFrom trait instead of using From, but that makes it a breaking API change.

The reason I did not make this faillible is because this actually improves the panic status we had before: Builder::new_current_thread().enable_all().build().expect() is basically the expansion of the #[tokio::main] macro. Before, all functions could panic since they all created their own runtime, now only the client creation could. But I do agree that making the initial creation faillible is much more manageable.

I'll make the change close to what you suggested, though I'll add another Error variant for this.

[spietika]

You are totally right. I overlooked the fact that, even though it is not as explicitly visible, of course #[tokio::main] has to create the runtime as well. But as said, now that it is relative easy to get rid of the panic altogether, it is worth doing so. Good idea to add a new Error variant to distinguish this error from others.

[Tuetuopay]

@spietika I just pushed the updated version. Overall the changes are minimal and I totally forgot that access to the blocking was behind commodity functions, so it's not a full breaking api change.

I did not create a new error variant because, as it happens, Builder::build() returns an std::io::Result, or in other words, a std::io::Error for which there was already a variant.

[spietika]

Looking good, clean and concise.