-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Add provider-style API to Context
#135051
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
Conversation
The job x86_64-gnu-llvm-18
failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
#21 exporting to docker image format
#21 sending tarball 26.1s done
#21 DONE 39.8s
##[endgroup]
Setting extra environment values for docker: --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure:
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
diff of stderr:
14 | |______^ `&mut Context<'_>` may not be safely transferred across an unwind boundary
15 |
16 = help: within `{async block@$DIR/async-is-unwindsafe.rs:12:19: 12:24}`, the trait `UnwindSafe` is not implemented for `&mut Context<'_>`
- = note: `UnwindSafe` is implemented for `&Context<'_>`, but not for `&mut Context<'_>`
18 note: future does not implement `UnwindSafe` as this value is used across an await
20 |
29 LL | fn is_unwindsafe(_: impl std::panic::UnwindSafe) {}
30 | ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_unwindsafe`
30 | ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_unwindsafe`
31
- error: aborting due to 1 previous error
+ error[E0277]: the type `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
+ --> $DIR/async-is-unwindsafe.rs:12:5
+ |
+ LL | is_unwindsafe(async {
+ | ^ ----- within this `{async block@$DIR/async-is-unwindsafe.rs:12:19: 12:24}`
+ | |
+ LL | |
+ LL | | use std::ptr::null;
+ LL | | use std::ptr::null;
+ LL | | use std::task::{Context, RawWaker, RawWakerVTable, Waker};
+ ... |
+ LL | | drop(cx_ref);
+ LL | | });
+ | |______^ `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
+ |
+ = help: within `{async block@$DIR/async-is-unwindsafe.rs:12:19: 12:24}`, the trait `UnwindSafe` is not implemented for `&mut (dyn Provider + 'static)`
+ note: future does not implement `UnwindSafe` as this value is used across an await
+ |
+ |
+ LL | let mut cx = Context::from_waker(&waker);
+ | ------ has type `Context<'_>` which does not implement `UnwindSafe`
+ ...
+ LL | async {}.await; // this needs an inner await point
+ | ^^^^^ await occurs here, with `mut cx` maybe used later
+ note: required by a bound in `is_unwindsafe`
+ |
+ |
+ LL | fn is_unwindsafe(_: impl std::panic::UnwindSafe) {}
+ | ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_unwindsafe`
+ error: aborting due to 2 previous errors
33
34 For more information about this error, try `rustc --explain E0277`.
35
35
Note: some mismatched output was normalized before being compared
- --> /checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:5
- | ^ ----- within this `{async block@/checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:19: 12:24}`
- LL | | //~^ ERROR the type `&mut Context<'_>` may not be safely transferred across an unwind boundary
- = help: within `{async block@/checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:19: 12:24}`, the trait `UnwindSafe` is not implemented for `&mut (dyn Provider + 'static)`
- --> /checkout/tests/ui/async-await/async-is-unwindsafe.rs:3:26
+ error[E0277]: the type `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
+ --> $DIR/async-is-unwindsafe.rs:12:5
+ |
+ |
+ LL | is_unwindsafe(async {
+ | ^ ----- within this `{async block@$DIR/async-is-unwindsafe.rs:12:19: 12:24}`
+ | |
+ LL | |
+ LL | | use std::ptr::null;
+ LL | | use std::ptr::null;
+ LL | | use std::task::{Context, RawWaker, RawWakerVTable, Waker};
+ ... |
+ LL | | drop(cx_ref);
+ LL | | });
+ | |______^ `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
+ |
+ = help: within `{async block@$DIR/async-is-unwindsafe.rs:12:19: 12:24}`, the trait `UnwindSafe` is not implemented for `&mut (dyn Provider + 'static)`
+ note: future does not implement `UnwindSafe` as this value is used across an await
+ |
+ |
+ LL | let mut cx = Context::from_waker(&waker);
+ | ------ has type `Context<'_>` which does not implement `UnwindSafe`
+ ...
+ LL | async {}.await; // this needs an inner await point
+ | ^^^^^ await occurs here, with `mut cx` maybe used later
+ note: required by a bound in `is_unwindsafe`
+ |
+ |
+ LL | fn is_unwindsafe(_: impl std::panic::UnwindSafe) {}
+ | ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_unwindsafe`
+ error: aborting due to 2 previous errors
The actual stderr differed from the expected stderr.
The actual stderr differed from the expected stderr.
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args async-await/async-is-unwindsafe.rs`
error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/async-await/async-is-unwindsafe.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/async-await/async-is-unwindsafe" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--edition=2018"
--- stderr -------------------------------
error[E0277]: the type `&mut Context<'_>` may not be safely transferred across an unwind boundary
##[error] --> /checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:5
|
|
LL | is_unwindsafe(async {
| ^ ----- within this `{async block@/checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:19: 12:24}`
| |
LL | | //~^ ERROR the type `&mut Context<'_>` may not be safely transferred across an unwind boundary
LL | | use std::ptr::null;
LL | | use std::ptr::null;
LL | | use std::task::{Context, RawWaker, RawWakerVTable, Waker};
LL | | drop(cx_ref);
LL | | });
| |______^ `&mut Context<'_>` may not be safely transferred across an unwind boundary
|
|
= help: within `{async block@/checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:19: 12:24}`, the trait `UnwindSafe` is not implemented for `&mut Context<'_>`
note: future does not implement `UnwindSafe` as this value is used across an await
|
|
LL | let cx_ref = &mut cx;
| ------ has type `&mut Context<'_>` which does not implement `UnwindSafe`
LL |
LL | async {}.await; // this needs an inner await point
| ^^^^^ await occurs here, with `cx_ref` maybe used later
note: required by a bound in `is_unwindsafe`
|
LL | fn is_unwindsafe(_: impl std::panic::UnwindSafe) {}
| ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_unwindsafe`
error[E0277]: the type `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
##[error] --> /checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:5
|
LL | is_unwindsafe(async {
| ^ ----- within this `{async block@/checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:19: 12:24}`
| |
LL | | //~^ ERROR the type `&mut Context<'_>` may not be safely transferred across an unwind boundary
LL | | use std::ptr::null;
LL | | use std::ptr::null;
LL | | use std::task::{Context, RawWaker, RawWakerVTable, Waker};
LL | | drop(cx_ref);
LL | | });
LL | | });
| |______^ `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
|
= help: within `{async block@/checkout/tests/ui/async-await/async-is-unwindsafe.rs:12:19: 12:24}`, the trait `UnwindSafe` is not implemented for `&mut (dyn Provider + 'static)`
note: future does not implement `UnwindSafe` as this value is used across an await
|
|
LL | let mut cx = Context::from_waker(&waker);
| ------ has type `Context<'_>` which does not implement `UnwindSafe`
...
LL | async {}.await; // this needs an inner await point
| ^^^^^ await occurs here, with `mut cx` maybe used later
note: required by a bound in `is_unwindsafe`
|
LL | fn is_unwindsafe(_: impl std::panic::UnwindSafe) {}
| ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `is_unwindsafe`
---
---- [ui] tests/ui/async-await/context-is-sorta-unwindsafe.rs stdout ----
error: test compilation failed although it shouldn't!
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/async-await/context-is-sorta-unwindsafe.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "-O" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/async-await/context-is-sorta-unwindsafe/a" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
--- stderr -------------------------------
error[E0277]: the type `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
##[error] --> /checkout/tests/ui/async-await/context-is-sorta-unwindsafe.rs:12:19
|
|
LL | unwind_safe::<Context<'_>>(); // test UnwindSafe
| ^^^^^^^^^^^ `&mut (dyn Provider + 'static)` may not be safely transferred across an unwind boundary
|
= help: within `Context<'_>`, the trait `UnwindSafe` is not implemented for `&mut (dyn Provider + 'static)`
note: required because it appears within the type `Option<&mut (dyn Provider + 'static)>`
note: required because it appears within the type `Context<'_>`
--> /rustc/FAKE_PREFIX/library/core/src/task/wake.rs:370:12
note: required by a bound in `unwind_safe`
--> /checkout/tests/ui/async-await/context-is-sorta-unwindsafe.rs:9:19
--> /checkout/tests/ui/async-await/context-is-sorta-unwindsafe.rs:9:19
|
LL | fn unwind_safe<T: UnwindSafe>() {}
error[E0277]: the type `(dyn Provider + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
|
|
LL | unwind_safe::<&Context<'_>>(); // test RefUnwindSafe
| ^^^^^^^^^^^^ `(dyn Provider + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
|
= help: within `Context<'_>`, the trait `RefUnwindSafe` is not implemented for `(dyn Provider + 'static)`
= note: required because it appears within the type `&mut (dyn Provider + 'static)`
note: required because it appears within the type `Option<&mut (dyn Provider + 'static)>`
note: required because it appears within the type `Context<'_>`
--> /rustc/FAKE_PREFIX/library/core/src/task/wake.rs:370:12
= note: required for `&Context<'_>` to implement `UnwindSafe`
note: required by a bound in `unwind_safe`
note: required by a bound in `unwind_safe`
--> /checkout/tests/ui/async-await/context-is-sorta-unwindsafe.rs:9:19
|
LL | fn unwind_safe<T: UnwindSafe>() {}
error: aborting due to 2 previous errors
For more information about this error, try `rustc --explain E0277`.
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.
Shouldn't it support more levels of inheritance?
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.
The intention is to traverse the chain of contexts/providers but now that you mention it I realized the code is not actually doing this.
I've got two concerns:
- Adding fields to
Context
increases its size even if the user doesn't enable the unstable features, or doesn't use nightly at all. The compiler is not able to optimize them out. AddingLocalWaker
tookContext
from 2 words to 4 words. This PR takes it from 4 words to 8 words. It's not much, but it's not nothing, and for embedded on the smallest chips it can make a difference. (there's people using async on chips with 2048 bytes of RAM). I worry that the unstable features we're adding toContext
will sit there indefinitely and not get stabilized, wasting bytes for no benefit. - The
Provider
API makes heavy use ofdyn
andTypeId
, which tends to generate rather slow+large code. Vtables are large, the indirect calls prevent inlining and optimizations which makes the surrounding code larger, TypeId generates large random 128bit constants and comparisons.- If this becomes the standard way to pass things through Context, it's not going to be great for embedded due to code size.
- I'd intuitively say it's going be slower than a thread local. It's going to be in the hot path (like, used for every wake or IO operation) so if it's slower runtimes will probably choose to keep using thread locals.
Adding fields to Context increases its size even if the user doesn't enable the unstable features, or doesn't use nightly at all.
I wonder how important this is. In my experience, Context
typically exists on the stack, so a few might get created during a task poll but they would not contribute to the sizes of generated futures.
The
Provider
API makes heavy use ofdyn
andTypeId
, which tends to generate rather slow+large code. [...] I'd intuitively say it's going be slower than a thread local.
Certainly if there are a lot of lookups for different interfaces. I think the best we could hope for is a single lookup being comparable to a thread local. In other words, ideally these two would have similar performance:
let rs: &RuntimeStuff = &RUNTIME_STUFF_LOCAL.with(|k| Rc::clone(k))); let rs: &RuntimeStuff = cx.request_ref::<RuntimeStuff>().unwrap();
Then at the very least a runtime could use the context to find its own stuff, even if that does little to help with the overall goal of runtime interop.
But maybe even this much is not true or possible with a provider-style API.
Thanks for the PR @jkarneges! This is the shape of an API I was hoping to see.
Adding fields to
Context
increases its size even if the user doesn't enable the unstable features, or doesn't use nightly at all. The compiler is not able to optimize them out. AddingLocalWaker
tookContext
from 2 words to 4 words. This PR takes it from 4 words to 8 words. It's not much, but it's not nothing, and for embedded on the smallest chips it can make a difference.
As a mitigation to the memory size concerns, it might be possible to embed the parent pointer inside the Provider object instead of storing two pointers in the Context. I can see how crafting an API around this is tricky, though.
I worry that the unstable features we're adding to
Context
will sit there indefinitely and not get stabilized, wasting bytes for no benefit.
I hear your concern and agree we should try to avoid this outcome. At the same time, I don't want to put too many barriers in front of experimentation.
The Provider API makes heavy use of dyn and TypeId, which tends to generate rather slow+large code. Vtables are large, the indirect calls prevent inlining and optimizations which makes the surrounding code larger, TypeId generates large random 128bit constants and comparisons.
I agree that we need to think about code size; I think for highly constrained environments we are going to have to rely on libraries having feature flags to remove runtime detection for reactors etc using this API. My guess is that those environments largely won't need the features (or ergonomics) enabled by this API, though I could be wrong about that.
I'd intuitively say it's going be slower than a thread local. It's going to be in the hot path (like, used for every wake or IO operation), so if it's slower runtimes will probably choose to keep using thread locals.
You may be right, but to challenge a bit, this would be on every suspension when we are waiting on IO. I don't see how the cost of doing an indirect call and comparing some integers is comparable to the time spent waiting. Yes that's time you could be spending on other stuff, but it still seems small by comparison.
Ultimately I think this is a question we could answer empirically by adding the API in nightly and allowing people to experiment. Perhaps we should put some parameters around it to make sure it doesn't stay forever without getting stabilized (e.g. as a strawman, revisit in 12 months).
r? libs
Perhaps the fields can be gated with a Cargo feature so they don't increase the size of Context
when not enabled? The only downside is it'd require -Zbuild-std-features
to use, but it'd still allow experimentation. (If we do this we should also do it for LocalWaker
)
@Dirbaio What about a default-enabled gate? My feeling is that we should lower the bar for experimentation as much as possible in the common case, where the extra bytes aren't a big deal.
how'd that work? there's no equivalent for default-features = false
in build-std, is it?
Also, there'd be no way to disable it on stable because build-std is unstable, unless we also cfg out the fields on the prebuilt std shipped with stable.
Did this get an ACP? I think it needs one, or at least a reason why it doesn't.
Ah, I thought ACPs were optional if a code author is willing to take the risk of their code getting rejected, which I'm fine with.
☔ The latest upstream changes (presumably #135937) made this pull request unmergeable. Please resolve the merge conflicts.
@jkarneges
Hey, thanks for your contribution.
From wg-triage. Have you been able to create that ACP?
Alright I've reworked the proposal text and put it over here: rust-lang/libs-team#552
This change enables
Context
to carry arbitrary extension data via an API similar to the one used byerror_generic_member_access
. It is intended to supersedecontext_ext
. It reusesRequest
and other types directly fromcore::error
, but it could be good to move those things to a common area if both the error & context usages are stabilized.Basic usage:
Currently,
Context
only carries aWaker
, but there is interest in having it carry other kinds of data. Examples include LocalWaker, a reactor interface, and multiple arbitrary values by type. There is also a general practice in the ecosystem of sharing data between executors and futures via thread-locals or globals that would arguably be better shared viaContext
, if it were possible.This change addresses some issues in the earlier proposed
context_ext
:context_ext
was intended to be stabilized without knowing what the most desirable data structure/mechanism for managing multiple extensions might be, in order to enable the ecosystem figure it out. Instead, this provider-style API is intended to be the desirable mechanism from the get-go. It supports returning multiple types, and overriding is possible by forwarding calls.Inheritance
Combinators that manage their own wakers will want to ensure the provider from an earlier context is carried over into any new contexts. For this, the
ContextBuilder::from()
method fromcontext_ext
can be used.Overriding extensions
Not only do we want a way for
Context
to carry multiple extensions, which the provider-style API solves, we also want a way for extensions to be added/overridden later on. For example, an executor may want to expose "reactor" & "spawner" interfaces from the top level, but then deeper in the call stack a "nursery" may want to override just the spawner (while allowing the reactor to still be accessible), and yet some other code path may want to introduce a "budgeter" for fair/weighted processing of some subset of work. These effects are achieved by having the context forward the provider calls to the parent provider after usingContextBuilder::from()
.Example overriding a "spawner":