-
Couldn't load subscription status.
- Fork 1.5k
Smol+async global executor 1.80 dev #3791
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
Smol+async global executor 1.80 dev #3791
Conversation
# Conflicts: # .github/workflows/sqlx.yml
|
I would prefer to deprecate |
Sure, I did not remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note as here: #3790 (review)
1f43f07 to
8d6377b
Compare
8c2c83c to
9b0cebb
Compare
35237ac to
c3e8f61
Compare
# Conflicts: # Cargo.toml
|
@martin-kolarik do you plan on addressing the CI failure and merge conflicts? |
I would like to address both. I will look at merge conflict first (today or on Monday) and we will see how tests changed. CI failure is linked to changes around |
# Conflicts: # Cargo.lock # Cargo.toml # rust-toolchain.toml # sqlx-cli/Cargo.toml # sqlx-core/Cargo.toml # sqlx-core/src/rt/mod.rs # sqlx-macros-core/src/lib.rs # sqlx-sqlite/src/connection/worker.rs
|
So, merged, fixed, and tests are running: the main issue of previous failures disappeared by itself, there certificate for localhost was expected. CI completion is prevented with error in build using minimal versions check, where |
|
Strictly speaking, this is a bug in The whole idea of the We could switch to I'd really like feedback from @iamjpotts here since he's the one who recommended adding the check to begin with (#3340). |
Yes, it is intended to be a correctness check on the declared minimums. From that standpoint the ci check is working as designed. The original issue was mentioned in #3118 (linked from #3340).
smol requires futures-lite 2.0, but in sqlx is optional. The fix is likely to be adding an explicit dependency on a transitive dependency, where the explicit dependency enforces the correct minimum. Possible workarounds to explore:
For either it would be a good idea to add a comment that the above is to resolve a problem with a transitive dependency (and which one, idk if it is |
Try to fix CI -Z minimal-versions check
I added dependency on |
This specific change lgtm though @abonander may have more fine grained feedback. |
| } | ||
|
|
||
| panic!("either the `runtime-async-std` or `runtime-tokio` feature must be enabled") | ||
| panic!("one of the `runtime-async-global-executor`, `runtime-async-std`, `runtime-smol`, or `runtime-tokio` feature must be enabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar - should be features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, should I change PR?
| // Work around `impl Socket`` and 'unability to specify test build cargo feature'. | ||
| // `connect_tcp_address` compilation would fail without this impl with | ||
| // 'cannot infer return type' error. | ||
| impl Socket for () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were any alternatives to this considered?
Why is this unreachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl Socket for () satisfies Rust type system, it is necessary at @260, when neither _rt-tokio nor _rt-async-io is selected. rustc cannot resolve impl Socket type without defining any. Also that's why the impl uses unreachable!, it is erased and unused during runtime.
The code has been added after discussing it in #3790 (comment), where shared implementation for both executors to resolve host names was suggested. I think it is fine, but it ended with impl Socket issue.
IMO there is alternative to use enum on connect_tcp_address return, but I expect it would bubble up through connect_tcp, which also has generic output, so enum would appear in many places. Using void impl on unit type seemed better to me.
|
|
||
| Ok(stream) | ||
| } else { | ||
| crate::rt::missing_rt(socket_addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a breaking behavior change. The existing behavior is to fall back to async_io if a Tokio runtime isn't available, which could be important for some use-cases.
Because of Cargo's feature unification, if a library using SQLx turns on the runtime-tokio feature, it's forced on for users of that library as well, whether they want it or not. The only way to turn it off is to patch the library.
This change would make a situation like that a hard blocker instead of mildly annoying.
|
I'm going to merge to avoid the need for any more back-and-forth, but I'm going to be making some adjustments in a follow-up PR. |
Thank you, unfortunately I was off during weekend so I did not respond to breaking change note. Feel free to ask for anything to be completed. |
* restore fallback to `async-io` for `connect_tcp()` when `runtime-tokio` feature is enabled * `smol` and `async-global-executor` both use `async-task`, so `JoinHandle` impls can be consolidated * no need for duplicate `yield_now()` impls * delete `impl Socket for ()`
* restore fallback to `async-io` for `connect_tcp()` when `runtime-tokio` feature is enabled * `smol` and `async-global-executor` both use `async-task`, so `JoinHandle` impls can be consolidated * no need for duplicate `yield_now()` impls * delete `impl Socket for ()`
* Sqlx-core: rename async_io dependency for async-std * Sqlx-core: simplify TimeoutError * Sqlx-core: code for async-global-executor * Sqlx: integrate async-global-executor feature * Note to unsafe * Step up MSRV as async-global-executor needs it * Sqlx-core: fix of unix socket build * Unsafe fixes, smol executor added * Workflow fix * Changes outside sqlx_rt * Cleanup conditional rt compilation * Warning * Add executors to test matrix * Fix of skipping code sqlite due to mismatch in cargo feature names * Smol executor isolated * Fix, reduce number of tests, remove async_std * Fix of test_block_on, regression * Format fixes * async-global-executor added * async-std changed to 1.13 * litemap, zerofrom requires rustc 1.81 * Fix of missing _sqlite in cargo.toml * Clippy lints * Clean up * Remove features combinations * Fixes after merge * Fix of compiling connect_tcp_address with both tokio + other executor selected * Try to fix CI -Z minimal-versions check Try to fix CI -Z minimal-versions check
* restore fallback to `async-io` for `connect_tcp()` when `runtime-tokio` feature is enabled * `smol` and `async-global-executor` both use `async-task`, so `JoinHandle` impls can be consolidated * no need for duplicate `yield_now()` impls * delete `impl Socket for ()`
async-global-executoradded as the fourth executor. The PR extends PR #3790.This is a breaking change --
async-global-executorhas MSRV 1.80, thereforesqlxwill have to be upgraded too.