Migrate to stable Future trait, async/await and Tokio 0.2 runtime #985
Conversation
Xanewok
force-pushed
the
migrate-to-stable-future
branch
from
067fd41
to
229bf34
Compare
|
This triggers a Clippy lint due to to many arguments (the existing function already seem to have 8, including @glandium would it be possible for you to take a stab at reviewing this this week (or get other contributors to review this to decrease the required review bandwidth)? |
|
This PR couldn't have arrived at a better time. We literally started using However, there is one new Windows-only issue that the changes here introduce, and it's somehow related to the Removing this section of code to effectively disable the proxy detection logic solves the problem. It seems like (only on Windows for some reason) the proxy logic ends up invoking the resolved Server trace logs below: Repro environment:
Repro steps:
|
|
@jblazquez could you file a separate issue with the information provided, it seems this is related to #666 rather than this PR |
|
Hi @drahnr, the issue does not occur with the code in master, only this PR. |
Xanewok
force-pushed
the
migrate-to-stable-future
branch
from
0c6ea8b
to
f79c6f6
Compare
|
Thanks @jblazquez for a detailed repro, I'll surely take a look! In the meantime I fixed the distributed compilation test failure and adapted remaining Windows code, so hopefully CI will finally be happy now. Some of the changes were tiny fixups to the big migration commit, so I combined those and force-pushed, while f79c6f6 changes the logic slightly and fixes a hang in dist. compilation. |
|
That's great to hear! I've been meaning to ping you once CI is green. PS I hope this can indirectly make League at least a bit better somehow 😎 |
|
@Xanewok sorry, I'm new to async Rust, so I don't understand why are you migrating to tokio 0.2, while tokio 1.2 is already available. |
The initial migration started at a time when tokio 1.0 was not released yet. It's anticipated that the migration to |
Tokio 0.2 is the first version that supports stable Moreover, Tokio 0.3 was only slightly changed in preparation for stable Tokio 1.0 release, both of which unfortunately skip support for named pipes in Windows (preferable IPC solution prior to Windows 10 1803, AF_UNIX sockets are available in later versions if needed) - that's pending on tokio-rs/tokio#3388. With that said, I'd expect the migration to work as follows:
While we're at it, it might be a good idea to get some data on Windows usage. @jblazquez what Windows build/version do you expect to run |
|
@Xanewok, in our case, it's Windows 10 for devs and Windows Server 2019 for build machines, both build 1809 or later. |
|
Gentle bump :) |
|
Unfortunately, this patch doesn't apply anymore. would it be possible to have a rebase? thanks |
Xanewok
force-pushed
the
migrate-to-stable-future
branch
2 times, most recently
from
0ed8d00
to
ccab162
Compare
|
Rebased and CI is somewhat happy (needs #1064). @sylvestre do you think it can be merged now? I'll work on the Tokio 1.0 upgrade right after. |
Co-authored-by: Igor Matuszewski <xanewok@gmail.com>
This fixes a bug introduced during the futures rewrite - we eagerly returned from the function rather than, as we did originally, falling back to compiling without proxy.
This fixes a run-time error otherwise encountered in tests/oauth.rs
Xanewok
force-pushed
the
migrate-to-stable-future
branch
from
df817a8
to
5478b91
Compare
|
Rebased over now-merged #1064, let's see if CI is fully green now. |
|
Terrific! Did you notice performance improvements or regressions? |
|
That's a good question! We didn't establish enough metrics internally to measure the performance accordingly; the original motivation for this PR was to migrate to stable |
FWIW, if you update the version of sccache used by Firefox to this revision in the toolchain definition, the build time metrics ought to be enough to surface any significant perf difference. sccache gets invoked a few thousand times in each Firefox build, it adds up! |
Followup on mozilla#985 to pull sccache the last little bit into tokio 1.0
Followup on mozilla#985 to pull sccache the last little bit into tokio 1.0
Followup on #985 to pull sccache the last little bit into tokio 1.0
Followup on mozilla/sccache#985 to pull sccache the last little bit into tokio 1.0
Followup on mozilla/sccache#985 to pull sccache the last little bit into tokio 1.0
Followup on mozilla/sccache#985 to pull sccache the last little bit into tokio 1.0
Builds on #945. Rebased and streamlined version of #946 (thanks to @Marwes and @drahnr for doing most of the work!).
This aims to be a somewhat minimal but complete upgrade to stable
Futuretrait and async/await.At first I tried to split the changes into respective commits but I quickly found that that e.g. migrating storages leads to
reqwestupgrade, which itself causes other places to be upgraded and so on. Rather than artificially setting up boundaries, migrating everything tofutures_03and.compat()everywhere and switching them back in following commits, I figured that this should be probably reviewed as a whole. Let me know if you really prefer to have it split more granularly somehow.Most notably:
tokioto0.2, which supports stableFutures.futures_03::executor::ThreadPoolto regulartokio::runtime::Runtime.Because Tokio-enabled I/O such as
tokio v0.2child processes or timeouts all implicitly now need to be spawned in a Tokio context, rather than creating a custom executor that usesThreadPoolbut polls relevant Tokio-related futures in a context of a separately running runtime, we use a single Tokio runtime everywhere configured with at least the same amount of worker threads as before and with enabledblockingfeature. This is designed for I/O-bound tasks (rather than CPU-bound ones) and so it fits the existing use case perfectly since we mostly usedspawn_fnfor operations related to disk I/O.reqwestto 0.10 (and by extensionhyperto0.13), supporting stableFutures.That's to easily support async/await in regular mode; in distributed compilation we use Tokio runtime now so it's also easier there to use
Future-enabled reqwest.async_traitcrate to migrate crucial traits likeStorage,Clientto async/await.This could be skipped in theory but that's mean sticking
Pin<Box<dyn Future<Output = XYZ> + Send + 'lifetimeas a return type everywhere, sinceasync fns in traits are not supported yet due to lack of GATs (see here)futures v0.1chains to async/await whenever possible.I tried to retain the existing semantics as much as possible and refrain from any non-trivial refactoring to ease the review and to help this land. Let me know if everything is in order and whether you want me to change anything here.