-
Notifications
You must be signed in to change notification settings - Fork 167
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
RUST-995 Add tokio-sync
feature flag
#578
Conversation
474baed
to
d7e38e4
Compare
src/runtime/mod.rs
Outdated
/// | ||
/// This will panic if called from a sychronous context when tokio is being used. | ||
#[cfg(any(feature = "sync", test))] | ||
#[cfg(test)] | ||
pub(crate) fn block_on<F, T>(self, fut: F) -> T |
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 method is now just used for configuring a fail point -- the sync code now calls block_on
directly on the static tokio runtime. It's possible we'll want to reconfigure this method but that can be done with a broader refactor in RUST-800.
src/runtime/mod.rs
Outdated
TokioCallingContext::Sync => None, | ||
}, | ||
Self::Tokio => { | ||
let handle = tokio::runtime::Handle::current(); |
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.
We can call current
directly because there should always be a tokio runtime going (either from the async caller or TOKIO_RUNTIME
). We can assume TOKIO_RUNTIME
has already been initialized here because the entry point for the execution of any async code is by calling TOKIO_RUNTIME.block_on
.
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.
I think that means the signature of this function can be changed to drop the Option
wrapper for the return type entirely.
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.
Looks good! A few smallish comments.
src/runtime/mod.rs
Outdated
TokioCallingContext::Sync => None, | ||
}, | ||
Self::Tokio => { | ||
let handle = tokio::runtime::Handle::current(); |
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.
I think that means the signature of this function can be changed to drop the Option
wrapper for the return type entirely.
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.
LGTM!
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.
Changes LGTM! Just need to sort out the lint failures and this should be good to go
.evergreen/check-rustdoc.sh
Outdated
@@ -5,4 +5,5 @@ set -o errexit | |||
. ~/.cargo/env | |||
cargo +nightly rustdoc -- -D warnings --cfg docsrs | |||
cargo +nightly rustdoc --no-default-features --features async-std-runtime -- -D warnings --cfg docsrs | |||
cargo +nightly rustdoc --no-default-features --features sync -- -D warnings --cfg docsrs | |||
cargo +nightly rustdoc --features sync -- -D warnings --cfg docsrs |
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.
I think we still need --no-default-features
here for the async-std sync variant. It looks like the check-rustdoc.sh
test is failing as a result.
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.
oh whoops, good catch
tokio-sync
feature flag
No description provided.