hwchen / secret-service-rs

Rust library for interfacing with Secret Service API
Apache License 2.0
72 stars 28 forks source link

zbus pulls in async-io #42

Open jeff-hiner opened 2 years ago

jeff-hiner commented 2 years ago

A lot of us are using tokio instead. There's a feature flag to build zbus with Tokio internals, but we would need to transitively expose this as a feature on secret-service.

brotskydotcom commented 1 year ago

Hi all, I'm the primary keyring-rs maintainer these days, and I'm working on a new (2.0) release that offers pluggable credential providers. I'm watching this thread with great interest because, if this crate is not going to offer a blocking interface out of the box, that's going to impact keyring pretty strongly. Either I will have to convert keyring to async - which would be a huge change for its clients - or I will have to write the blocking wrapper myself in keyring.

While I am a big believer in async, especially for I/O-bound multi-processing systems, I don't think that credential stores are a very high priority for async. Most credential stores on other platforms (and even secret-service on dbus, for that matter) offer sync-only interfaces, and the usage patterns for credential stores tend to be read-once use-many which means there aren't a lot of blocking calls past the startup. So I would encourage you to think about maintaining a blocking interface for backward compatibility in this release.

In any event, please do keep this thread posted on plans, because I don't want to release a 2.0 of keyring and then immediately have to release a 3.0 due to async. Thanks!

zeenix commented 1 year ago

usage patterns for credential stores tend to be read-once use-many which means there aren't a lot of blocking calls past the startup.

I don't know all the relevant details so maybe I'm missing something essential but in general:

Either I will have to convert keyring to async - which would be a huge change for its clients - or I will have to write the blocking wrapper myself in keyring.

Sure but keep in mind that blocking wrappers are only needed for re-usability reasons as it's just block_on(async_func()). i-e you can keep your code entirely sync and just wrap every async call in a block_on call.

brotskydotcom commented 1 year ago

If you're doing IPC, you've blocking calls and hence async makes sense.

It's secret service that's doing IPC, not the secret service client who is trying to access the credential. Credential providers on non-Linux platforms are generally OS-provided utilities that wrap up all the system calls in a nice neat synchronous package, and even Linux has a kernel-provided keyring with a sync interface.

In my experience, credentials are accessed most often by human-driven clients (whether GUI or command-line) and all of those clients tend to be synchronous because credential access/modification is always thought of as "atomic without a mutex". Sure, there may be high volume services that are async and need access to the credentials, but it's easy for them to access synchronous calls and they almost always are caching the credentials in memory for fast access in subsequent actions.

My point here is not that credential providers shouldn't offer asynchronous interfaces - I think all providers should - but that credential providers that have offered synchronous interfaces in the past are doing that for a reason, and they should be considerate of their (many) existing clients who haven't even thought about async or how to use it (and are not likely to for a long time).

Sure but keep in mind that blocking wrappers are only needed for re-usability reasons as it's just block_on(async_func()). i-e you can keep your code entirely sync and just wrap every async call in a block_on call.

I'm sure you know well that it's not quite that simple or that easy: the mere fact that there is both tokio and async-std (and the entire discussion above) is a pretty clear demonstration that linking to async code (even wrapped async code) forces the client to learn more about async and its assorted dependencies than they were expecting to. If I expose choices around async frameworks to keyring-rs clients, they are not going to be happy campers, especially if they use other frameworks that make different choices. What I'm hoping here is that the folks doing the secret-service release, who are the ones that are really adopting async zbus, are the ones who can work through all these issues and find a way of hiding them from clients such as keyring.

In any event, I'm looking forward to the release, because it comes at just the right time in the keyring lifecycle. We are moving to plug-in providers, and the new secret-service would be a great provider to have in our toolbox (even if we use the older, dbus-based, synchronous one as the default for backward compatibility).

zeenix commented 1 year ago

I'm sure you know well that it's not quite that simple or that easy: the mere fact that there is both tokio and async-std (and the entire discussion above)

Just to be clear again, there is no "async-std" involved and it never was.

What I'm hoping here is that the folks doing the secret-service release, who are the ones that are really adopting async zbus, are the ones who can work through all these issues and find a way of hiding them from clients such as keyring.

I don't think this crate can magically hide all that for you (that's what I tried very hard to do in zbus but didn't quite succeed). If you have a look at the relevant PR, you'll see that the plan is to provide you with features to choose between tokio and async-io backends of zbus.

The using code can then also choose to proxy the features but abstracting over the block_on implementations remains simple enough. You can basically just take the code from zbus.

complexspaces commented 1 year ago

The tokio-support branch seemed to work well in my work codebase, so support for zbus 3 and async/await have landed in the main branch. I'll be cutting a 3.0 release this week if all goes well.

brotskydotcom commented 1 year ago

@zeenix Yes, thanks for all your work on zbus, and I agree you've done a great job reducing the client overhead to a simple choice between tokio and async-io. My concern is just that I now have to pass that choice on to the keyring-rs clients, and they are unlikely to know what it means :). But that's just the way things always are at this point when async is introduced.

@complexspaces Thanks for all your work on integrating zbus. If @jkhsjdhjs can get his PR prepared in time for you to include it in your 3.0 release, that would be terrific, as it will allow me to pick up the enhancements for use in the upcoming keyring release.

zeenix commented 1 year ago

@zeenix Yes, thanks for all your work on zbus, and I agree you've done a great job

You're welcome.

My concern is just that I now have to pass that choice on to the keyring-rs clients, and they are unlikely to know what it means :). But that's just the way things always are at this point when async is introduced.

I hear you and I wish this wasn't the case but unfortunately you will have to either give the choice to your users or make it for them. The only compromise I could think of is providing the choice but choosing the default for them. Which is what both zbus and secret-service is doing.

complexspaces commented 1 year ago

With 3.0 (finally) on crates.io (including zbus's newer Tokio integrations), is anyone opposed to closing this issue as complete?

brotskydotcom commented 1 year ago

I agree that the 3.0 release fixes this issue.