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

Usage from async contexts #54

Open
SafariMonkey opened this issue Apr 20, 2021 · 3 comments
Open

Usage from async contexts #54

SafariMonkey opened this issue Apr 20, 2021 · 3 comments

Comments

@SafariMonkey
Copy link

I have been looking into laying the groundwork for moving some Vault-dependent microservices to Rust, likely using async. I was somewhat surprised to see that this client still exposes a totally blocking API, even though e.g. reqwest's primary API is now blocking, and (I imagine) many of the applications which would want to talk to Vault would be using async. As it stands, an async application would have to spawn_blocking the request, which would then internally use async anyway (in the reqwest implementation).

Until #51, I believe writing an async implementation and basing the (current) synchronous API on it could have been done in a non-breaking way, but it seems that ship has now sailed for 2.x. Such an approach would now require two separate implementations.

What are your thoughts on catering to the async use case?

@SafariMonkey
Copy link
Author

SafariMonkey commented Apr 26, 2021

I made an attempt at an async conversion here. It is a breaking change (obviously), but it's almost all just adding .await and async, plus some minor workarounds to extract the response body, as the async body doesn't implement Read or AsyncRead. The tests all passed as soon as they built (and once an async runtime compatibility issue was resolved).

This is not a PR as such, just me demonstrating what an async-ifying change could look like.

@ChrisMacNaughton
Copy link
Owner

While I'm conceptually not against doing async, this library is likely to stay with sync in the HTTP client, even on the next major version, as I intend to use ureq to minimize dependencies of the crate. Ureq's stance on async is that it does, and always will, use synchronous IO for operations as it dramatically simplifies the requirements and dependencies.

@ehiggs
Copy link

ehiggs commented Feb 13, 2023

The reasons for not doing async in the Ureq repo make sense for them. But Vault is often used by backend services to manage private keys. These backend services manage many connections already using async. Keeping the vault client sync means calling:

let cloned_client = client.clone(); // client is in an arc to allow it to be cloned since it doesnt support cloning itself.. :(
let cloned_key = key.clone();
let result = tokio::task::spawn_blocking( move || {cloned_client.get_secret(cloned_key}).await??;

What a pain the bum for each call to grab a secret!
Also, most people using web stuff have reqwests already installed and the interface is very well done and easy to use. So adding a whole other stack seems like adding more dependencies than you hope to save here. So I hope you reconsider - or whomever maintains this reconsiders.

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

No branches or pull requests

3 participants