Skip to content

Allow download counts to fail to be updated #1675

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

Merged
merged 2 commits into from
Mar 19, 2019

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Mar 18, 2019

This ensures that the download endpoint still works even if counting the
download fails. The main case that we expect failure to occur is when
the application is in read only mode. However, even if an unexpected
failure occurs, we still want cargo build to succeed. Counting
downloads is always considered optional -- it's much more important that
people's builds succeed than having accurate download stats.

Note that we still require a database connection from the pool. In
theory, we could allow getting the version ID to fail as well, and just
blindly redirect to S3 no matter what, and rely on a 404 happening
there. However, this could result in successful builds in the event that
the index is out of sync with our DB, since we don't routinely clean up
the S3 bucket. Our current test behavior says that we 404 instead of
redirecting, so I've left things as is.

Once sfackler/r2d2#73 is released, my plan is to
try to get a DB connection with a shorter timeout, and if we were able
to get a DB connection, do our current behavior, but if that fails just
blindly redirect to S3. We don't expect getting a connection to fail
unless we're being DDOS'd, nor do we expect requests to this endpoint
for crates that don't exist (since they should only come for things in
the index).

Fixes #1387

This ensures that the download endpoint still works even if counting the
download fails. The main case that we expect failure to occur is when
the application is in read only mode. However, even if an unexpected
failure occurs, we still want `cargo build` to succeed. Counting
downloads is always considered optional -- it's much more important that
people's builds succeed than having accurate download stats.

Note that we still require a database connection from the pool. In
theory, we could allow getting the version ID to fail as well, and just
blindly redirect to S3 no matter what, and rely on a 404 happening
there. However, this could result in successful builds in the event that
the index is out of sync with our DB, since we don't routinely clean up
the S3 bucket. Our current test behavior says that we 404 instead of
redirecting, so I've left things as is.

Once sfackler/r2d2#73 is released, my plan is to
try to get a DB connection with a shorter timeout, and if we were able
to get a DB connection, do our current behavior, but if that fails just
blindly redirect to S3. We don't expect getting a connection to fail
unless we're being DDOS'd, nor do we expect requests to this endpoint
for crates that don't exist (since they should only come for things in
the index).

Fixes rust-lang#1387
@bryanburgers
Copy link
Contributor

I didn't test it locally, but this looks good to me.

@sgrif
Copy link
Contributor Author

sgrif commented Mar 19, 2019

@bors r=jtgeibel

@bors
Copy link
Contributor

bors commented Mar 19, 2019

📌 Commit c9a2b20 has been approved by jtgeibel

@bors
Copy link
Contributor

bors commented Mar 19, 2019

⌛ Testing commit c9a2b20 with merge b84b91a...

bors added a commit that referenced this pull request Mar 19, 2019
Allow download counts to fail to be updated

This ensures that the download endpoint still works even if counting the
download fails. The main case that we expect failure to occur is when
the application is in read only mode. However, even if an unexpected
failure occurs, we still want `cargo build` to succeed. Counting
downloads is always considered optional -- it's much more important that
people's builds succeed than having accurate download stats.

Note that we still require a database connection from the pool. In
theory, we could allow getting the version ID to fail as well, and just
blindly redirect to S3 no matter what, and rely on a 404 happening
there. However, this could result in successful builds in the event that
the index is out of sync with our DB, since we don't routinely clean up
the S3 bucket. Our current test behavior says that we 404 instead of
redirecting, so I've left things as is.

Once sfackler/r2d2#73 is released, my plan is to
try to get a DB connection with a shorter timeout, and if we were able
to get a DB connection, do our current behavior, but if that fails just
blindly redirect to S3. We don't expect getting a connection to fail
unless we're being DDOS'd, nor do we expect requests to this endpoint
for crates that don't exist (since they should only come for things in
the index).

Fixes #1387
@bors
Copy link
Contributor

bors commented Mar 19, 2019

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing b84b91a to master...

@bors bors merged commit c9a2b20 into rust-lang:master Mar 19, 2019
@sgrif sgrif deleted the sg-resilient-crate-download branch March 25, 2019 21:25
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.

4 participants