Skip to content

Add smol as an executor #3790

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Conversation

martin-kolarik
Copy link

This PR adds smol as a third async executor, to overcome async_std obsolescence.

async_std remains working, but it is replaced with smol in CI, to reduce number of run tasks.

Adding smol is not a breaking change.

Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General note: because async-std and smol share a lot of the same constituent crates, it would save us a lot of code deduplication to just use those crates directly where possible (namely async-io).

Comment on lines +238 to +268
} else if #[cfg(feature = "_rt-smol")] {
use smol::net::resolve;
use smol::Async;
use std::net::TcpStream;

let mut last_err = None;

// Loop through all the Socket Addresses that the hostname resolves to
for socket_addr in resolve((host, port)).await? {
let stream = Async::<TcpStream>::connect(socket_addr)
.await
.and_then(|s| {
s.get_ref().set_nodelay(true)?;
Ok(s)
});
match stream {
Ok(stream) => return Ok(with_socket.with_socket(stream).await),
Err(e) => last_err = Some(e),
}
}

#[cfg(not(feature = "_rt-async-std"))]
{
crate::rt::missing_rt((host, port, with_socket))
// If we reach this point, it means we failed to connect to any of the addresses.
// Return the last error we encountered, or a custom error if the hostname didn't resolve to any address.
Err(match last_err {
Some(err) => err,
None => io::Error::new(
io::ErrorKind::AddrNotAvailable,
"Hostname did not resolve to any addresses",
),
}
.into())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this code duplication isn't really necessary. Since async-std and smol both use async-io, the async-std block should work for both with a few tweaks. And then the async-global-executor impl can share the same code path as well (async-net just wraps async-io).

The difference between async_std::net::ToSocketAddrs and smol::net::resolve() seems significant, but in fact, they both simply invoke std::net::ToSocketAddrs in a blocking task:

Funnily enough, Tokio does the exact same thing: https://docs.rs/tokio/latest/src/tokio/net/addr.rs.html#219-221

They all also have a fast path for when the hostname can be parsed as an IP address that skips the blocking task.

This would be easy enough to implement ourselves in a single code path using our own spawn_blocking abstraction. Then only runtime-specific part would be deciding which TcpStream type to use.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented, but it looks that TLS test are failing after this changed. Do not know why so far, even going back does not help.

@martin-kolarik
Copy link
Author

General note: because async-std and smol share a lot of the same constituent crates, it would save us a lot of code deduplication to just use those crates directly where possible (namely async-io).

The issue is that async_std and smol use different async_io versions. As sqlx needs async_io too (socket...), having single async_io in cargo.toml would result to semver conflicts. I managed to overcome this with creating two async_io with distinct name aliases, but then the code had to be duplicated. Is there any strategy you would prefer that I should follow?

@abonander
Copy link
Collaborator

The issue is that async_std and smol use different async_io versions.

As of async-std 1.13 it also uses async-io ^2. The existing situation technically already has a Semver issue because if someone upgrades to async-std 1.13, SQLx will still use async-io 1 and spawn a redundant I/O driver thread. So I'd just say go ahead and upgrade async-io to 2.0 and increase the minimum version of async-std to 1.13.

This is likely going to be delayed until the 0.9.0 release anyway since a whole new runtime feature is a bit much to add in a point release.

@martin-kolarik
Copy link
Author

martin-kolarik commented Apr 13, 2025

Next question: if 0.9.0 is a target, will sqlx's MSRV be lifted to +1.81? If yes, then the split between smol and async-global-executor would be unnecessary and only single branch/PR would be enough.

@martin-kolarik
Copy link
Author

martin-kolarik commented Apr 13, 2025

After all changes (in async-global-executor PR) a) check using minimal version is failing, for the first sight on strange error in async_fs, b) unit tests are failing because rustc cannot deduce impl Socket type in new connect_tcp_address. I am bit confused, because the test runs with --all-features, so I would rather expect incompatible types error.

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

Successfully merging this pull request may close these issues.

2 participants