Skip to content

Support for smol runtime [wip] #348

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

Closed
wants to merge 17 commits into from

Conversation

hwchen
Copy link
Contributor

@hwchen hwchen commented May 29, 2020

Hi, I made a first pass at supporting the smol runtime.

Although it basically works, it still needs cleanup and testing, so I wanted to check and see if you're interested in supporting smol, and if this implementation is in the right direction.

Note: there is no smol::test attr, could see if smol_potat will add it.

@mehcode
Copy link
Member

mehcode commented May 29, 2020

What's the advantage here over async-std which is now a wrapper over smol as I understand it? I'm not opposed to supporting smol directly but I don't think that's possible (you've pulled in some other crates to help as well).

@hwchen
Copy link
Contributor Author

hwchen commented May 29, 2020

It's hard to say that either async-std or smol have an advantage; they expose different APIs, and I happen to like smol's API and minimalist philosophy.

I'm not sure what you mean by not being able to support smol directly? Do you mean without other dependencies?

In terms of deps, unless I made a mistake I think I only pulled in pin_project_lite, perhaps this can be removed.

Just also want to say that I understand the burden of maintenance from adding another runtime, so i don't want to push this on you if it won't be valuable. Thanks for developing sqlx, it's worked great for me so far.

@mehcode
Copy link
Member

mehcode commented May 29, 2020

Sorry for not being clear. My point is as async-std is a wrapper around smol, is there any point to provide both runtimes? If we just drop direct async-std support is anyone affected by that change?

@hwchen
Copy link
Contributor Author

hwchen commented May 29, 2020

Hmmm... I'm not entirely sure. I'll take a look and see.

@hwchen
Copy link
Contributor Author

hwchen commented May 30, 2020

So I've asked for some clarification in the smol discord and did some experiments.

Since async-std spins up a smol runtime automatically, a library that supports smol can be run within an async-std context. I tested by running this smol-only branch of sqlx in an async-std app, and it worked. I would also expect all the async-std tests to work if the feature was switched to runtime-smol.

Running an async-std library within a smol app should also work. At the moment, async-std would spawn a few extra threads of smol runtime. For most use cases, I don't think this is a critical issue, but it would be a bit disappointing as I assume users of smol want more control over the runtime.

There may be other considerations on the interaction between async-std and smol that I'm not aware of. For example, the switch to smol led to this issue (looks like fix is already in): async-rs/async-std#798 . Depending on the direction you want to take, we could ask for more clarification from smol and async-std maintainers.

@hwchen
Copy link
Contributor Author

hwchen commented May 30, 2020

I updated the PR to just use smol under the hood of the runtime-async-std feature. This way, you can just choose between using smol or the current async-std route, since it probably doesn't make sense to support both explicitly. (At least as long as async-std closely tracks smol)

@mehcode
Copy link
Member

mehcode commented May 31, 2020

Any thoughts on this @dignifiedquire ?

I'm leaning towards dropping direct usage of async-std as it seems to have the most upside ( people with just smol can use SQLx natively and people with async-std can use SQLx without much of a difference ).

@dignifiedquire
Copy link
Contributor

I would suggest an alternative path, which I think could bring the best of both worlds. By default use async-std with default features disabled (once 1.6.1 is realeased, there is a bug atm), which will not include the runtime and start it, but it will give you access to the building blocks, allowing you to avoid implementing the shims that are in this PR.

This would not give you block_on, but I would strongly suggest not using smol::run for this, as that brings you into issues if things are already using other wrappers, but use smol::block_on.

@dignifiedquire
Copy link
Contributor

it is important to also note that currently sqlx does not actually work with smol, only [email protected] :(

ref smol-rs/smol#136

@hwchen
Copy link
Contributor Author

hwchen commented Jun 6, 2020

Thanks @dignifiedquire. That sounds fine to me, it's true that the shim is just selected snippets of async-std anyways.

Can you just say a bit more about This would not give you block_on..., I'm not sure about the context? Would that be for implementing smol compatibility in sqlx, or for end-users of slqx?

@mehcode
Copy link
Member

mehcode commented Jun 7, 2020

By default use async-std with default features disabled (once 1.6.1 is realeased, there is a bug atm), which will not include the runtime and start it, but it will give you access to the building blocks, allowing you to avoid implementing the shims that are in this PR.

That's an awesome idea. I think this is definitely the way to go.


However, we use block_on in a proc-macro which means we will bring in the default async-std features until cargo has a stable release of the new feature resolver.

This is still a great thing to do though as it enables nightly rust users to opt-out of the default features.


@hwchen Thank you so much for driving this. I'm going to close this PR for now as the intended direction is to simply remove default features from async-std in core (once that bug in smol is fixed).

@mehcode mehcode closed this Jun 7, 2020
@hwchen
Copy link
Contributor Author

hwchen commented Jun 12, 2020

@mehcode thanks for considering this! And @dignifiedquire thanks for the feedback!

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.

3 participants