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

Tracking issue for using hyper web server in production #2204

Closed
6 of 7 tasks
jtgeibel opened this issue Feb 20, 2020 · 5 comments · Fixed by #3436
Closed
6 of 7 tasks

Tracking issue for using hyper web server in production #2204

jtgeibel opened this issue Feb 20, 2020 · 5 comments · Fixed by #3436
Labels
A-backend ⚙️ C-tracking-issue Category: A tracking issue for an RFC, an unstable feature, or an issue made of many parts

Comments

@jtgeibel
Copy link
Member

jtgeibel commented Feb 20, 2020

There are several open issues that need to be addressed before we can enable hyper in production:

  • 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
  • Work in progress: Release some improvements to the conduit crates that allow for a better implementation in conduit-hyper when serving static files. conduit-rust/conduit@0deff5b
  • Fix sub-categories. : comes through as %3A, resulting in a 404. Bump to latest release of conduit-hyper #2533
  • Switch to USE_HYPER=1 on production!

Cleanup:

  • Switch to hyper by default, so that local development aligns with production.
  • Eventually remove dependency on civet.

Old

  • I want to add a callback that gives the application a chance to handle a request async if the count of background threads is exceeded. This would potentially let us handle a download redirect instead of rejecting those requests due to exceeding our thread count capacity. Update: Because the download endpoint needs at least a read-only database connection, this isn't a short-term goal.
jtgeibel added a commit to jtgeibel/crates.io that referenced this issue Feb 22, 2020
When using `conduit-hyper` to serve requests, if the max thread count is
reached then new requests are rejected immediately rather than blocking.
The fallback value for `SERVER_THREADS` is increased to ease local
testing when setting `USE_HYPER=1`.

Ref: rust-lang#2204
@jtgeibel
Copy link
Member Author

@carols10cents I've just opened #2212 to address the first task.

For the 2nd task I've added a test upstream in conduit-rust/conduit-hyper@f7f53b8 to ensure this works and doesn't regress. I was able to replicate your results locally when adding a delay to /me and /me/updates. By then loading the /dashboard frontend URL I saw that the requests were issued serially such that a blank page is shown for 20 seconds until the user data is loaded from /me and then the dashboard data is requested causing another delay.

By trying the same experiment with /me and /summary and then loading /, the two requests are sent in parallel and they both complete after just a single duration.

bors added a commit that referenced this issue Feb 24, 2020
Increase the default thread count to 5 in development

When using `conduit-hyper` to serve requests, if the max thread count is
reached then new requests are rejected immediately rather than blocking.
The fallback value for `SERVER_THREADS` is increased to ease local
testing when setting `USE_HYPER=1`.

cc #2204
r? @carols10cents
@Turbo87 Turbo87 added A-API C-tracking-issue Category: A tracking issue for an RFC, an unstable feature, or an issue made of many parts labels Apr 1, 2020
jtgeibel added a commit to jtgeibel/crates.io that referenced this issue May 24, 2020
The latest upstream changes should allow us to try enabling hyper in
production. All tasks tracked in rust-lang#2204 should be completed.
bors added a commit that referenced this issue May 25, 2020
Bump to the latest conduit-hyper

The latest upstream changes should allow us to try enabling hyper in
production. All tasks tracked in #2204 should now be completed.

r? @JohnTitor
bors added a commit that referenced this issue May 27, 2020
Bump to latest release of conduit-hyper

This upstream release now percent decodes the path component but not the
query string. This behavior aligns with the `civet` server. The known
difference is that `conduit-hyper` does a lossy utf8 conversion while
`civet` panics on invalid utf8 and closes the connection immediately.

This seems like a good compromise of matching the existing behavior as
closely as possible without copying the panic. I looked into fixing the
panic in `civet`, however the `to_str_slice` function returns an
`Option<&str>` and there is nowhere to store a newly allocated `String`
so changing the behavior there is not practical.

r? @JohnTitor
cc #2204

conduit-rust/conduit-hyper@v0.3.0-alpha.3...v0.3.0-alpha.4
bors added a commit that referenced this issue Jul 10, 2020
Switch to hyper as the default web server

This is changed so that running `cargo run --bin server` locally uses
the same web server that we are running in production. The `civet`
server can be selected by setting the `WEB_USE_CIVET` environment
variable to any value (including an empty string).

r? @JohnTitor
cc #2204
@Turbo87
Copy link
Member

Turbo87 commented Nov 11, 2020

@jtgeibel @carols10cents to we have a timeline for the last part of this issue ("Eventually remove dependency on civet.")?

I assume that this would potentially also improve our compile times a little bit :)

@Turbo87
Copy link
Member

Turbo87 commented Nov 11, 2020

and in case it wasn't obvious: I'm happy to open a PR for this once we think it's time to say goodbye to civet 😉

@jtgeibel
Copy link
Member Author

I assume that this would potentially also improve our compile times a little bit :)

civet is a pretty simple wrapper around civet-sys which is a C codebase and compiles extremely quickly. It doesn't add much to our compile times, especially when compared to tokio and hyper.

The main reason I'd like to keep this around at the moment is to confirm there are no differences in response times. Over the last few months the 50th percentile response time on the dashboard seems to have grown a bit, and have a bit more variance. There was a very slight difference between civet and conduit-hyper when I first switched, but I haven't ruled out that things could have regressed since then.

I think it's most likely that we're just seeing more traffic and may want to increase the number of database connections. And I'm talking an average of 13ms with a few ms of variance here. Basically, I've seen a slight change over the last 6 months and it hasn't been enough to really worry me. However, in case this is a trend, it would be nice to have another server implementation that we already know works on production. It probably isn't a factor, but it seems nice to have the option just in case.

@Turbo87
Copy link
Member

Turbo87 commented Nov 11, 2020

However, in case this is a trend, it would be nice to have another server implementation that we already know works on production. It probably isn't a factor, but it seems nice to have the option just in case.

That sounds to me like we won't remove this in the foreseeable future. Should we close this issue then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-tracking-issue Category: A tracking issue for an RFC, an unstable feature, or an issue made of many parts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants