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

Don't force HTTP/2 #26

Merged
merged 3 commits into from
Sep 6, 2023
Merged

Conversation

Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Sep 6, 2023

Checklist

  • I have read the Contributor Guide
  • I have read and agree to the Code of Conduct
  • I have added a description of my changes and why I'd like them included in the section below

Description of Changes

Do not force HTTP/2. Corporate transparent proxies often do not support it, but cargo audit needs to run in these environments.

After this change HTTP/2 will be used where available, but the download will not fail if it's not.

Related Issues

rustsec/rustsec#988

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Sep 6, 2023

Curiously I'm not seeing a regression in fetch times for the sparse index even when I force HTTP 1.x. You'd think it'd be slower because we're missing out on multiplexing, but apparently not.

@Jake-Shadle
Copy link
Member

I'd be interested to see if this affects throughput, as I believe now it has to/might do an extra round-trip if the remote server is HTTP/2?

Regardless, it's a shame that antiquated corporate environments negatively? Impact all other users.

@Jake-Shadle
Copy link
Member

I believe it does a round-trip to upgrade, but maybe it caches that for the client so doesn't impact each unique request.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Sep 6, 2023

I agree this is unfortunate. It would be nice to try HTTP/2 first, but corporate proxies just blackhole the request instead of replying with "505", so that would cause every request to first time out for 10 seconds and only then fall back to HTTP 1.1. So I'm afraid that's untenable.

Since it's possible to supply a custom Client, use cases where avoiding a roundtrip is preferable to wide compatibility can force HTTP/2 in the client builder. I don't think that should be the default though - things actually working is generally preferable to slightly higher performance.

@Jake-Shadle Jake-Shadle merged commit 30d2762 into EmbarkStudios:main Sep 6, 2023
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.

2 participants