Skip to content
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

fix: Sccache dist tests broken after bump to tokio 1.21 and later #1563

Merged
merged 2 commits into from Jan 28, 2023

Conversation

Xuanwo
Copy link
Collaborator

@Xuanwo Xuanwo commented Jan 16, 2023

It's a long-term problem that sccache dist tests failed after bumping tokio.

After a bisect of all commits between tokio 1.20.1 to 1.21.0, I confirmed that this problem was introduced in tokio-rs/tokio#4840. The PR itself is simple and correct, but it can lead to problems in hyper (reqwest affected too).

After this PR, hyper can't reuse connections between different runtimes. To address this problem, we should ensure all requests sent by hyper are from the same runtime. But it's a huge change based on our current design. So in this PR, I disabled the keep-alive of reqwest to make sure everything just works as before.

Changes that are included in this PR:

  • Disable keep-alive in hyper side
    • NOTE: we always set keep-alive: Close by hand, so this change won't affect of dist server's perf.
  • Use the new image for testing (ubuntu 22.04)
    • Use openssl 3.0 and a new version of glibc, which makes it possible to test locally on modern systems.
  • Use localhost with network host instead of docker container IP
    • Make it possible to test with complex and different docker setups. For example, use podman-docker without the docker0 network bridge.
  • Bump the version of tokio and tokio-utils
  • Smaller our lock scope

Future Plan:

  • Should we migrate to an async web framework instead?
  • Maybe we need to init the async runtime first and always stick to the same one?
  • We should not spawn a new thread to use reqwest::blocking::Client.

Close #1541
Close #1543

@Xuanwo Xuanwo marked this pull request as ready for review January 16, 2023 08:27
@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Jan 16, 2023

cc @sylvestre and @messense for review

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Jan 16, 2023

Ooops, freebsd still failing for dist test failed.

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023

Codecov Report

Base: 30.90% // Head: 30.86% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (a9edcf5) compared to base (412f7e0).
Patch coverage: 14.81% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1563      +/-   ##
==========================================
- Coverage   30.90%   30.86%   -0.04%     
==========================================
  Files          48       48              
  Lines       16555    16298     -257     
  Branches     7923     7755     -168     
==========================================
- Hits         5116     5031      -85     
- Misses       6057     6061       +4     
+ Partials     5382     5206     -176     
Impacted Files Coverage Δ
src/dist/http.rs 0.00% <ø> (ø)
src/test/mock_storage.rs 26.92% <ø> (ø)
tests/dist.rs 100.00% <ø> (ø)
src/server.rs 30.33% <12.50%> (-2.03%) ⬇️
tests/harness/mod.rs 39.28% <33.33%> (-3.40%) ⬇️
tests/system.rs 24.08% <0.00%> (-6.69%) ⬇️
tests/sccache_cargo.rs 31.69% <0.00%> (-2.53%) ⬇️
src/compiler/clang.rs 52.06% <0.00%> (-1.24%) ⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Jan 16, 2023

image

Seems it's an unstable test.

@sylvestre
Copy link
Collaborator

Many thanks

@sylvestre sylvestre merged commit 3c4fe05 into mozilla:main Jan 28, 2023
@sylvestre
Copy link
Collaborator

@Xuanwo it seems that test ubuntu-22.04 rust 1.60.0 dist-server is failing
https://github.com/mozilla/sccache/actions/runs/4032885110/jobs/6933042098


Download action repository 'actions/upload-artifact@v3' (SHA:0b7f8abb1508181956e8e162db84b466c27e18ce)
Run ./.github/actions/artifact_failure
Run killall sccache || true
sccache-dist: no process found
tar: .: file changed as we read it
Error: Process completed with exit code 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants