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

Proposal: remove runtime features and async-std support. Still using async-std? Please make yourself heard! #1669

Closed
abonander opened this issue Feb 3, 2022 · 18 comments
Labels

Comments

@abonander
Copy link
Collaborator

abonander commented Feb 3, 2022

TL;DR: we want to drop existing runtime features and use Tokio everywhere in SQLx.

If you are still using runtime-async-std-[native-tls, rustls], please give this issue a 👎 so we know you're out there!

The runtime-actix-[native-tls, rustls] features would also go away, but as long as Actix is still based on Tokio, SQLx should continue to work with it just fine. Currently, these features exist purely to try to prevent confusion, but they also seem to cause a good bit of confusion at the same time, so getting rid of them doesn't seem controversial to me.

Background and Motivation

SQLx is rather unique in its class as an async library that needs heavy runtime support but still tries to support multiple runtimes.
Most async libraries are either entirely runtime-agnostic, or committed solely to one runtime.

SQLx was initially envisioned as an async-std-only library. We had intended to use it with Tide as our web backend stack at Launchbadge. Very soon after release, the request to support Tokio became very popular (heh, we initially labeled that issue wontfix), being echoed both on Discord and on Reddit, and so we added Tokio in 0.2.0.

We soon added support for the Actix runtime (which is Tokio under the hood) as well, and also settled on Actix-web for our web stack for various reasons. (We're considering Axum for new projects, which is also Tokio-based.)

Since then, supporting multiple runtimes has turned out to be quite the maintenance burden.

We had to create a whole subcrate just to provide a unified interface, using mutually exclusive Cargo features because we conditionally re-export items from one of tokio, async-std or actix-rt under conflicting names, and so can't have more than one enabled at once. This has caused a lot of issues, especially because we force the user to pick a runtime even when they would prefer not to (like if they're just trying to use the derive macros).

The mutual exclusion is also necessary to pick between tokio-native-tls, tokio-rustls, async-std-native-tls and async-std-rustls even though they're all more or less identical, just linking different crates. We believe this could also be resolved by Cargo weak features which will be stabilized in Rust 1.60, but is still worth noting.

We have 69 CI passes, which take around 15 minutes to run, thanks to having to cover all the possible combinations of the databases, database versions, runtimes and TLS providers that we support, although to keep from going overboard we only test the oldest and newest versions we support of each database.

Getting rid of runtime features in general and committing solely to Tokio would simplify SQLx, and usage of it, quite a bit.

These days, Tokio is the superior option.

  • Tokio is actively maintained, while async-std hasn't had a release in 5 months. Last commit to async-std/master was 2 months ago.
  • Tokio has a larger ecosystem with several killer apps:
    • hyper, reqwest, actix-web, rusoto, axum, tokio-tungstenite, tonic, redis, tokio-postgres
    • async-std has just Tide and Surf, I think, and those have stagnated as well.
  • Tokio has a stronger community:
    • Tokio is more popular, with 8x more downloads on crates.io compared to async-std.
    • Tokio has 540 contributors on Github compared to async-std's 126.

Implementation

  • Drop the runtime-* matrix of features.
  • Remove the sqlx-rt crate and replace usages of sqlx_rt with tokio in other crates.
  • Add native-tls and rustls features which would no longer have to be mutually exclusive; if both are enabled, native-tls should take priority.

What would happen to async-std users?

If there's still a significant cohort of async-std SQLx users, we don't just want to kick them to the curb. In order to make our decision, we want to try to gauge how many of them there are which is why you should 👎 this issue if you would be negatively affected by this change.

There's also async-compat which can be used to make Tokio-based crates work on async-std by using a single-threaded Tokio runtime spawned into a background thread.

It should work well enough for SQLx, with the exception of Pool. Because PoolConnection wants to spawn a task on-drop to make sure the buffers are flushed and the connection is still usable, just wrapping a future that uses Pool or PoolConnection in Compat won't be sufficient, as the drop handler will be run after the tokio::runtime::EnterGuard is dropped. It shouldn't panic as the drop handler already checks if the runtime is available or not, but it does mean that every connection checked out from Pool would be immediately closed after use, which kind-of defeats the purpose.

The solution I'm thinking is to allow providing a tokio::runtime::Handle to use in PoolOptions, but getting a handle to the Tokio runtime that async-compat spawns would require depending on Tokio in your application, as it doesn't provide direct access to it:

let pool: sqlx::PgPool = Compat::new(async {
    sqlx::pool::PoolOptions::new()
        .tokio_handle(tokio::runtime::Handle::current())
        .connect("<DATABASE_URL>")
        .await
}).await?;

Alternatively, this could work without depending on Tokio (I'm thinking we'd provide both):

let pool: sqlx::PgPool = Compat::new(async {
    sqlx::pool::PoolOptions::new()
        .use_current_tokio()
        .connect("<DATABASE_URL>")
        .await
}).await?;
@Kinrany
Copy link

Kinrany commented Feb 3, 2022

This may be a good moment to enumerate features that sqlx uses that aren't currently runtime-agnostic.

@joshtriplett
Copy link
Contributor

I'm using sqlx in production with async-std and Tide, and I'd like to avoid additionally introducing a Tokio runtime on top of that. I do use the Pool mechanism.

With my libs team hat on: we're working on introducing traits and other facilities into the standard library that would help you be more runtime-agnostic. I'm also hopeful that we'll gain a runtime-agnostic way to spawn a future. I'd be happy to work with you to figure out what you'd need to make that viable. I completely understand that having to maintain an indirection layer for runtime compatibility is a pain; I'm hoping that you'll consider waiting a little longer and helping us give you the language and library features you need to no longer need that indirection layer.

@abonander
Copy link
Collaborator Author

We've been hearing about efforts to add more async interfaces to the standard library since 1.39.0. Are there any concrete developments as of late? I don't really have the energy to keep track of all the RFCs anymore.

@mitsuhiko
Copy link

@abonander Even if there are efforts for this I doubt that they will be adopted at this point. The common example here is that people are not expecting tracing to work and I do not see this in std, yet tokio is very much considering this for it's own spawning system.

Personally I'm very interested in the outcome of this as I'm also myself hoping that consolidation around tokio makes it easier to build async APIs going forward. In particular supporting tracing is something I am very interested in.

@joshtriplett
Copy link
Contributor

@abonander Yes, wg-async is actively working on standardizing AsyncBufRead/AsyncRead/AsyncWrite, AsyncIterator (formerly Stream), and several other things. (I'm expecting that spawn will take longer as it depends on an additional language feature.)

@paolobarbolini
Copy link
Contributor

paolobarbolini commented Feb 3, 2022

With my libs team hat on: we're working on introducing traits and other facilities into the standard library that would help you be more runtime-agnostic. I'm also hopeful that we'll gain a runtime-agnostic way to spawn a future. I'd be happy to work with you to figure out what you'd need to make that viable. I completely understand that having to maintain an indirection layer for runtime compatibility is a pain; I'm hoping that you'll consider waiting a little longer and helping us give you the language and library features you need to no longer need that indirection layer.

Having experienced this mainly from lettre I think the main issue for me is the fact that there are too many implementations of async runtime agnostic APIs. I don't mind having a wrapper that calls tokio::spawn or async_std::spawn based on which runtime the user chooses.
For example I see some crates depend on futures-channel just because they want to be runtime agnostic, but tokio also provides a channel implementation, so I would be lying if I didn't admit that in the back of my mind I feel a bit annoyed by that.
I think: I'm already "invested" in the tokio ecosystem, so why would that crate instead decide to depend on a different implementation with no clear benefits. I imagine this issue would be even worse from the eyes of an async-std user: why would this crate depend on tokio just for the channel implementation. "Just use Y instead".

This was just a very simple example, but we hit something similar in #1668. What do we do? Use tokio semaphores when tokio is enabled and something else for async-std? That would then complicate the implementation though, because they have different APIs and introduce even more variables that wouldn't be there if we could just agree on a general implementation for doing this kind of stuff that everybody would see as mature as their runtime. Even if you don't agree with the above "fanboy-ism" this is clearly an issue.

Sorry if this sounds like a rant, I love tokio and have nothing against async-std, but I feel like this is also what other people think. At least that's what I gather when reading issues around other people's projects.

@abonander
Copy link
Collaborator Author

abonander commented Feb 3, 2022

As far as what SQLx needs, obviously the most important part is an async TcpStream which is not runtime-agnostic unless you spin up your own I/O driver, at which point you're halfway to inventing your own runtime anyway. Eventually, we do want to support blocking I/O as well for applications that don't want to take on the complexity of async nor need the scalability.

We also need integration with TLS libs. Currently we support rustls and native-tls because both of them have their merits in this application: the former for those who want pure Rust and a consistent open-source implementation across platforms, and the latter for wider support as a lot of people want to use SQLx to connect to older databases with TLS versions and ciphers that RusTLS declines to support. We of course recommend that people upgrade to the latest versions for security reasons but sometimes in this business you just have to work with what you've got.

Right now we have to use a different crate for each (runtime, TLS lib) pair, minus Actix as that just uses the Tokio stuff. We've considered rolling our own versions but it goes back to the same issues of runtime-dependence and the lack of standard traits. We've also been asked to support BoringSSL although there's not a lot of weight being thrown behind that at the moment.

Pool internally uses task spawning and timeouts to implement its various features. Task spawning is not strictly necessary if we have something like AsyncDrop or promises to make sure connections are properly recycled and not left in an inconsistent state. Unexpected cancellations have been a significant papercut for us, because it's incredibly easy to forget that every .await point is a place where execution is likely to diverge and never return.

We'd need to figure out a different strategy for the idle reaper, but it could theoretically just be a thread instead of a task.

Yes, wg-async is actively working on standardizing AsyncBufRead/AsyncRead/AsyncWrite

Again, that's great but it's really not anything new. It's been talked about in some form or another since async hit stable over two years ago. Do you have any idea of a concrete timeline on this stuff? Is it waiting for something, like async fn in traits, which is still waiting on GATs which is waiting on... something?

Moreover, even if or when that stuff is standardized, and assuming it makes its way to stable Rust relatively quickly, it would rely on both Tokio and async-std updating to use it which would mean a 2.0 release for both, which may or may not be a trivial refactor, and which would take a while to trickle out through the ecosystem anyway. Given the current pace of development, I don't see that happening in less than a year. How long do you want us to wait? I'm not being sarcastic here, it's a legitimate question.

@skade
Copy link

skade commented Feb 3, 2022

I know several major async-std users that I can sadly not disclose :/. Particularly, async-std is ported to proprietary platforms. Most of those users actively pick async-std bc. of its different runtime behaviour.

Not sure how many of them would actually need sqlx support, but I see the major issue here as much as everyone else: a failure to provide even basic common interfaces.

That being said, I'm actually in favor of running IO for something like SQLx in its own library reactor rather than hooking into a global application runtime. smol is a perfect runtime for that. It also eases the pain of integrating into other runtimes (I'm aware of more runtimes than just async-std and tokio out there). Spinning up a thread for the database driver also has the advantage that it's easier to understand which tasks are scheduled there and makes it harder to accidentally block them.

From my perspective, a major pain here is the belief that we need to schedule everything on one runtime. It makes libraries and the main application very entangled while rusts async/.await makes it very easy to await across runtimes.

@c410-f3r
Copy link
Contributor

c410-f3r commented Feb 3, 2022

Just reiterating that this current behavior also affects libraries that have to pick a runtime to make builds compile

sqlx/sqlx-rt/src/lib.rs

Lines 1 to 25 in 135d16a

#[cfg(not(any(
feature = "runtime-actix-native-tls",
feature = "runtime-async-std-native-tls",
feature = "runtime-tokio-native-tls",
feature = "runtime-actix-rustls",
feature = "runtime-async-std-rustls",
feature = "runtime-tokio-rustls",
)))]
compile_error!(
"one of the features ['runtime-actix-native-tls', 'runtime-async-std-native-tls', \
'runtime-tokio-native-tls', 'runtime-actix-rustls', 'runtime-async-std-rustls', \
'runtime-tokio-rustls'] must be enabled"
);
#[cfg(any(
all(feature = "_rt-actix", feature = "_rt-async-std"),
all(feature = "_rt-actix", feature = "_rt-tokio"),
all(feature = "_rt-async-std", feature = "_rt-tokio"),
all(feature = "_tls-native-tls", feature = "_tls-rustls"),
))]
compile_error!(
"only one of ['runtime-actix-native-tls', 'runtime-async-std-native-tls', \
'runtime-tokio-native-tls', 'runtime-actix-rustls', 'runtime-async-std-rustls', \
'runtime-tokio-rustls'] can be enabled"
);

Where in fact such feature should be chosen by end-users applications or testing environments. Here goes two known workarounds:

  1. SeaORM replicates the same set of features (https://github.com/SeaQL/sea-orm/blob/e63d4631559c0fac369f0a94fcabebaaf36f1d29/Cargo.toml#L55-L94).
  2. I have a project that supports several optional DB backends and an exclusive private feature was introduced just for SQLX (https://github.com/c410-f3r/oapth/blob/8e8dc7f26f97a2235e4729a3992917b293e990cf/oapth/Cargo.toml#L61).

So yeah, big thumbs up for a single runtime or anytime else that won't restrict downstream libraries.

@joshtriplett
Copy link
Contributor

I'm very much in favor of smol as well, though I'd still rather have a single runtime in the entire program (where I can control things like how many threads to run in a thread pool and similar whole-program concerns) rather than having multiple runtimes.

@abonander Standardized async I/O traits are waiting on a subset of async-fn-in-traits, which doesn't depend on GATs or anything else complex; a simple version that's sufficient for most of what folks need is in the accepted RFC 3185, and being implemented now. At this point I think we're talking months, not years.

@djc
Copy link
Contributor

djc commented Feb 3, 2022

The proposal makes sense to me, although

if both are enabled, native-tls should take priority

I'd flip that default. As a rustls maintainer, I'm biased of course, but I think selecting improved security (including TLS 1.3 support) over compatibility with old database versions would make sense.

@rrichardson
Copy link

rrichardson commented Feb 3, 2022

I just want to state one assumption out loud for confirmation/rejection by @abonander :

If/when async executor support arrives in std I'd assume that SqlX would move to support that. So this move to singularly support the Tokio runtime would be unwound at that point?

If one were heavily invested into one of the other async runtimes one might be motivated to assist the AsyncWg/Library/Compiler teams in order to speed that effort along.

@Spindel
Copy link

Spindel commented Feb 3, 2022

We're using async-std + Pool in production, but we don't use the TLS features (sqlite rather).

@abonander
Copy link
Collaborator Author

Given the outpouring of support for async-std, we're considering extracting sqlx-rt to a separate crate to act as an abstraction layer that crates like SQLx can build on. I have some ideas for how we can make the runtime features additive and also support not having any runtime enabled.

We would also probably need to implement a TLS crate built on it that acts in the same vein but for RusTLS, native-tls and maybe BoringSSL as well.

@djc RusTLS would probably be the default recommendation but if people have trouble connecting with it, one of the troubleshooting steps would involve enabling native-tls to see if that fixes things.

@Sytten
Copy link

Sytten commented Feb 4, 2022

There are already a few existing crates for that, would be nice to see some consolidation in that space so library maintainers all use the same thing...

@abonander
Copy link
Collaborator Author

Neither crate provides a unified interface for async I/O types, just the very basic runtime capabilities (block_on, spawn).

@yerke
Copy link
Contributor

yerke commented Feb 7, 2022

@abonander Thank you and your team for being so open minded and flexible.

@abonander
Copy link
Collaborator Author

Well, we're just trying to be considerate. If we had never implemented Tokio support or started out on Tokio rather than async-std then we probably wouldn't be having this conversation. We easily could have just closed #6 as wontfix and moved on like so many other projects did.

Still, I'll reiterate that this really shouldn't be necessary. The efforts of the Rust libs team to provide standard async I/O traits should be lauded, but it's coming in quite late and the onus will still be on Tokio and async-std to actually adopt them, and do so in a timely fashion. We've also only arrived at this point after almost three years of tribalism and what can only be described as vendor lock-in, which really shouldn't happen in an open-source ecosystem.

Moving on from this, we're going to work on an abstraction layer unifying the APIs of Tokio and async-std which is more efficient than async-compat and that isn't coupled from SQLx so other crates can benefit as well.

I strongly encourage the Tokio and async-std maintainers to be ready to adopt the async I/O traits when they land in std which should hopefully make everyone's jobs easier in the future.

Thanks everyone for your feedback.

@launchbadge launchbadge locked as resolved and limited conversation to collaborators Feb 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests