Skip to content

fix(registry): HttpRegistry block_until_ready returns early when work is still pending#14694

Merged
bors merged 2 commits into
rust-lang:masterfrom
arlosi:fix-extra-blocking
Oct 15, 2024
Merged

fix(registry): HttpRegistry block_until_ready returns early when work is still pending#14694
bors merged 2 commits into
rust-lang:masterfrom
arlosi:fix-extra-blocking

Conversation

@arlosi

@arlosi arlosi commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

What does this PR try to resolve?

block_until_ready is returning early when there are still pending transfers. This leads to extra restarts of the resolver, impacting performance.

In the existing implementation:

  • handle_completed_downloads runs and there are no completed downloads
  • multi.perform() is called and completes new downloads, with no pending transfers remaining
  • Since there are no items remaining_in_multi, the function returns early

This fixes the issue by reordering the calls to multi.perform(), handle_completed_downloads, then the completion check.

How should we test and review this PR?

A test is added that uses cargo's tracing to show the number of blocking calls. First commit shows existing behavior, second commit shows fix.

Originally based on the PR #14680 by @x-hgg-x. The ordering fix in this PR also avoids an additional block_until_ready call for retried failed downloads.

@rustbot

rustbot commented Oct 15, 2024

Copy link
Copy Markdown
Collaborator

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-registries Area: registries A-sparse-registry Area: http sparse registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2024
@Eh2406

Eh2406 commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

does the code

self.add_sleepers()?;
also need adjustment?

@weihanglo

Copy link
Copy Markdown
Member

does the code

self.add_sleepers()?;

also need adjustment?

Seem pretty likely, though this is downloading the package so the perf gain may not be as significant as registry index one.

@weihanglo weihanglo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍🏾
Good either with or without #14694 (comment).

@arlosi

arlosi commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

does the code

self.add_sleepers()?;

also need adjustment?

The current implementation looks correct to me. It doesn't have the same bug with early returning when there is still work to be done.

My notes on how it's working:

loop {
- wait()
  - wait_for_curl() 
     loop {
     - add_sleepers() [self.sleeping -> self.pending (may not move all, depending on timing)]
     - multi.perform() [self.pending -> self.results]
     }
  - self.pending.remove()
  - self.sleeping.push() [for failed downloads that need retry]
}

@arlosi

arlosi commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

@bors r=weihanglo

@bors

bors commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

📌 Commit 8db2192 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2024
@bors

bors commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

⌛ Testing commit 8db2192 with merge 79a491a...

@bors

bors commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 79a491a to master...

@bors bors merged commit 79a491a into rust-lang:master Oct 15, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
Update cargo

7 commits in 8c30ce53688e25f7e9d860b33cc914fb2957ca9a..cf53cc54bb593b5ec3dc2be4b1702f50c36d24d5
2024-10-15 16:43:16 +0000 to 2024-10-18 13:56:15 +0000
- feat: Stabilize MSRV-aware resolver config (rust-lang/cargo#14639)
- Help with `[patch.crates.io]` (rust-lang/cargo#14700)
- test: Migrate publish snapshotting to snapbox (rust-lang/cargo#14642)
- Bump to 0.85.0; update changelog (rust-lang/cargo#14695)
- Fix typo in faq.md (rust-lang/cargo#14696)
- fix(registry): `HttpRegistry` `block_until_ready` returns early when work is still pending (rust-lang/cargo#14694)
- fix(resolver): avoid cloning when iterating using RcVecIter (rust-lang/cargo#14690)
@rustbot rustbot added this to the 1.84.0 milestone Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-registries Area: registries A-sparse-registry Area: http sparse registries S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants