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

Switch to async/await for bin/server and tests #1864

Merged
merged 19 commits into from
Feb 20, 2020

Conversation

jtgeibel
Copy link
Member

@jtgeibel jtgeibel commented Oct 10, 2019

This PR series converts the future combinations to async/await syntax in the server binary (when USE_HYPER=1 is set) and the recording HTTP proxy used in tests.

This change requires the update of several dependencies at once (see the list below). It is not strictly necessary to update reqwest at this time, but I'm doing so in this series to avoid the build time regression of pulling in hyper 0.12, hyper 0.13 and their associated transitive dependencies.

Blockers:

  • async/await is stable in 1.39.0 🎉
  • Bump the rust release in RustConfig
  • Stable release of underlying stack
    • futures 0.3.1
    • tokio 0.2.9
    • hyper 0.13.1 and hyper-tls 0.4.1
    • reqwest 0.10.1
    • conduit-hyper 0.2.0

@rust-highfive
Copy link

r? @sgrif

(rust_highfive has picked a reviewer for you, use r? to override)

@bors
Copy link
Contributor

bors commented Oct 30, 2019

☔ The latest upstream changes (presumably #1876) made this pull request unmergeable. Please resolve the merge conflicts.

@jtgeibel jtgeibel force-pushed the update/conduit-hyper branch from 231fb9d to 732b484 Compare November 15, 2019 02:50
@bors
Copy link
Contributor

bors commented Nov 22, 2019

☔ The latest upstream changes (presumably #1899) made this pull request unmergeable. Please resolve the merge conflicts.

@jtgeibel jtgeibel force-pushed the update/conduit-hyper branch from ae781c1 to 683168f Compare December 19, 2019 04:07
@jtgeibel jtgeibel marked this pull request as ready for review December 19, 2019 04:19
@jtgeibel
Copy link
Member Author

🎉 I've rebased this branch and now consider it ready for review. 🎉

There is currently a missing feature in conduit-hyper which should be resolved before we try rolling out USE_HYPER=1 to production again. That can be handled in a future PR and I think we can land this now for the improvements to the test harness.

r? @carols10cents

@rust-highfive rust-highfive assigned carols10cents and unassigned sgrif Dec 19, 2019
@bors
Copy link
Contributor

bors commented Jan 4, 2020

☔ The latest upstream changes (presumably #2072) made this pull request unmergeable. Please resolve the merge conflicts.

@jtgeibel jtgeibel force-pushed the update/conduit-hyper branch from 683168f to 248f65a Compare January 4, 2020 17:39
@jtgeibel
Copy link
Member Author

jtgeibel commented Jan 4, 2020

Rebased to resolve merge conflicts, and added a commit to switch from git master to the now released reqwest v0.10.0.

@bors
Copy link
Contributor

bors commented Jan 5, 2020

☔ The latest upstream changes (presumably #2066) made this pull request unmergeable. Please resolve the merge conflicts.

@jtgeibel jtgeibel force-pushed the update/conduit-hyper branch from 248f65a to 4d11d4b Compare January 13, 2020 22:47
@jtgeibel
Copy link
Member Author

I'd like to merge this PR before bumping any more dependencies in Cargo.toml, to avoid messy rebasing conflicts.

I've just pushed a new commit that resolves the remaining known issue. The server will now limit the max number of background threads. If the max thread count is exceeded by a new request, that request will be rejected with a 503 Service Unavailable response. This ensures that we don't end up with an effectively unbounded queue of threads waiting to obtain a database connection. Of course, hyper is still only used when USE_HYPER=1 is set.

@jtgeibel
Copy link
Member Author

The build failure is a regression on nightly and is already reported as rust-lang/rust#68264.

Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looking and need to test but wanted to save the one comment I have so far :)

Cargo.toml Outdated Show resolved Hide resolved
@carols10cents
Copy link
Member

I'm having trouble signing in locally when I'm on this branch rebased on master at 7072305 (and I verified that when I'm on 7072305 locally I can sign in), even without USE_HYPER=1.

It appears to be failing when requesting the user info from GitHub; the request is returning 403. I'm able to print out the Authorization token value that I'm trying to use locally (println!("auth secret = {}", auth.secret()); as in here), and use it in a curl request to GitHub that returns successfully:

curl -H "Accept: application/vnd.github.v3+json" -H "Authorization: token [redacted]" https://api.github.com/user

Soooo 🤷‍♀ ???

There is an new FIXME added to `server.rs` when using `USE_HYPER=1`
until I implemenet a max blocking theads limit in `conduit-hyper`.
We do not currently enable hyper in production so I think it makes
sense to land the changes to the `record.rs` test harness.
If a new request causes the maximum thread count to be exceeded, the
request will be rejected with a 503 Service Unavailable response.
@jtgeibel jtgeibel force-pushed the update/conduit-hyper branch from d8e77f8 to 0b06ca8 Compare February 6, 2020 05:03
@jtgeibel
Copy link
Member Author

jtgeibel commented Feb 6, 2020

@carols10cents excellent catch! So it turns out reqwest no longer sets a default user-agent header. I've rebased and then added a commit which sets the user-agent to our URL and updates the tests to always check for this value.

I'm pushing the changes to staging now, and will also set USE_HYPER=1 there.

The tests are also updated to ensure that the user-agent is set for
outgoing requests to GitHub and S3.
@jtgeibel jtgeibel force-pushed the update/conduit-hyper branch from 0b06ca8 to b5e1209 Compare February 6, 2020 05:47
@bors
Copy link
Contributor

bors commented Feb 13, 2020

☔ The latest upstream changes (presumably #2186) made this pull request unmergeable. Please resolve the merge conflicts.

@carols10cents
Copy link
Member

2 issues we investigated tonight:

  • The default number of threads used in development mode needs to be increased to make the site usable locally (I tried 4 and it seemed to work ok)
  • Using thread::sleep in a controller function appeared to be blocking other requests, and this wasn't the expected behavior when using tokio::task::spawn_blocking

@carols10cents
Copy link
Member

If those two issues get tracked somewhere and block turning on hyper for everyone everywhere, feel free to r=me :)

@jtgeibel
Copy link
Member Author

I've opened #2204 as a tracking issue for any remaining blockers for enabling USE_HYPER=1 in production.

@bors r=carols10cents

@bors
Copy link
Contributor

bors commented Feb 20, 2020

📌 Commit f724f05 has been approved by carols10cents

@bors
Copy link
Contributor

bors commented Feb 20, 2020

⌛ Testing commit f724f05 with merge 28216b3...

@bors
Copy link
Contributor

bors commented Feb 20, 2020

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing 28216b3 to master...

@bors bors merged commit 28216b3 into rust-lang:master Feb 20, 2020
@jtgeibel jtgeibel deleted the update/conduit-hyper branch May 11, 2020 23:19
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.

5 participants