futures migration to 0.3 #946
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* change (CI): initial CI * change (CI): initial CI * change (CI): add clippy and allow failure
PRs labelled `autorebase:opt-in` will be rebased automatically after updates in `master`.
In order to avoid timestamp presence in static lib files, making them non-deterministic. rust-lang/cc-rs@555e773 mozilla/sccache#197
This brings in line with the original implementation that used `futures_01::future::select_all`, which waited for any of the underlying futures to be ready - either server completes or we receive an explicit shutdown signal or we were idle for long enough.
Otherwise the Tokio timer creation panics because there's no Tokio timer instance running when spawning via `futures::executor::ThreadPool`
* Spawn root compilation task in Tokio context Since Tokio 0.2, spawning the `tokio::process::Command` must be done in the Tokio context, so make sure to spawn the compilation root task in the Tokio runtime that's now available in the `SccacheService`. This fixes a hang in the `sccache_cargo` integration test. * Mark ClientToolchains::put_toolchain as synchronous It seems entirely synchronous, as it doesn't hit any await points and additionally seems to be trigger-happy with mutex locks in inner calls, so better be safe and prevent the future reader from thinking that this function is async-safe. * Prepare cache stuff to use Tokio as its I/O-bound task pool * Prepare to prune ThreadPool entirely in favor of Tokio The reason for that being not to mix the Tokio runtime and the `futures` executor. While it's reasonable and possible to do so, I believe there's bigger benefit for now to stick to a single executor. It would be great if we could keep the code using 'vanilla' `futures` crate but Tokio support is so baked in (e.g. child process harness) that it seems that it'd require a lot more change for questionable benefit in this case. Doing so may yield another benefit - currently, there seems to be a lot of disk I/O being done on a dedicated, blocking thread pool. It's possible, now that we use a single executor, to use Tokio facilities for async I/O if this proves to be faster when supported properly by the native environment at some point (e.g. with `io_uring` on Linux). * Prefer using Tokio runtime for dist. HTTP client * Prune futures::executor::ThreadPool * Prefer using Tokio timer again * Bring expected test output in line with upstream repository This lets test pass locally on Ubuntu 20.10 with: gcc (Ubuntu 10.2.0-13ubuntu1) 10.2.0 Ubuntu clang version 11.0.0-2 * Cherry-pick of CI: switch to staging image before CI is green; fix versions output 85286f9 Co-authored-by: Denis P <denis.pisarev@parity.io>
* Fix some types when not using dist-client feature * Pull required features for Tokio 0.2 This fixes a `--no-default-features` build since other dependencies incidentally pulled the relevant features themselves. * Fix compilation when using only azure feature * Fix compilation when using only dist-server feature
Most notably prefer std::future::Future over futures::Future since the std variant is here to stay
Let's stick to the upstream version. We seem to only care about starting and keeping the child server process alive until the end of the function, prefer the std variant for simplicity and to avoid footguns such as `Runtime::enter` that we had to use here.
The async `get_dist_status` called `get_status` that was locally blocking in a freshly created `Runtime`. Instead, use the already present `Runtime` instead creating it from scratch and mark `get_status` appropriately as async.
The only reason that's there is because the compiler is too conservative and treats borrows as alive longer than they actually are (even with explicit `drop`). Work around that by creating an owned future instead, returned from the `resolve_proxied_executable`. It's worth noting that `async_trait` returns a `Pin<Box>`ed future with the 'async lifetime, where we have &'async self as the method receiver. Because of this, we manually construct the future ourselves with the 'static lifetime.
|
Superseded by #985 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is mostly a PR to raise awareness that the migration to futures 0.3 is WIP, see paritytech/cachepot#31 for current status and remaining steps.
Now this is a rather big chunk of changes, partially non-atomic changes over multiple commits.
Ref #945