Skip to content

Remove pending requests from ready_requests#6625

Merged
mergify[bot] merged 17 commits intosigp:unstablefrom
ackintosh:remove-pending-requests-from-ready-requests
Feb 10, 2026
Merged

Remove pending requests from ready_requests#6625
mergify[bot] merged 17 commits intosigp:unstablefrom
ackintosh:remove-pending-requests-from-ready-requests

Conversation

@ackintosh
Copy link
Member

Issue Addressed & Proposed Changes

The pending requests in delay_requests move to ready_requests before being returned by poll_ready. One request is returned per poll_ready call, so some pending requests might remain in ready_requests. We need to remove the pending requests from ready_requests, just like delay_requests, when a peer disconnects.

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Nice find Akihito! submitted #6634 to ease it

mergify bot pushed a commit that referenced this pull request Feb 19, 2025
We don't need to store `BehaviourAction` for `ready_requests` and therefore avoid having an `unreachable!` on #6625.
Therefore this PR should be merged before it
@ackintosh ackintosh marked this pull request as ready for review February 19, 2025 22:56
@ackintosh ackintosh added ready-for-review The code is ready for review Networking labels Feb 19, 2025
pawanjay176 pushed a commit to pawanjay176/lighthouse that referenced this pull request Feb 21, 2025
We don't need to store `BehaviourAction` for `ready_requests` and therefore avoid having an `unreachable!` on sigp#6625.
Therefore this PR should be merged before it
eserilev pushed a commit to eserilev/lighthouse that referenced this pull request Mar 5, 2025
We don't need to store `BehaviourAction` for `ready_requests` and therefore avoid having an `unreachable!` on sigp#6625.
Therefore this PR should be merged before it
@mergify
Copy link

mergify bot commented May 19, 2025

This pull request has merge conflicts. Could you please resolve them @ackintosh? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 19, 2025
@mergify mergify bot closed this Jun 18, 2025
@mergify
Copy link

mergify bot commented Jun 18, 2025

Hi @ackintosh, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

@mergify mergify bot added the stale Stale PRs that have been inactive and is now outdated label Jun 18, 2025
# Conflicts:
#	beacon_node/lighthouse_network/src/rpc/self_limiter.rs
@ackintosh ackintosh reopened this Aug 10, 2025
@mergify mergify bot added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 10, 2025
@ackintosh ackintosh removed the stale Stale PRs that have been inactive and is now outdated label Aug 10, 2025
# Conflicts:
#	beacon_node/lighthouse_network/src/rpc/self_limiter.rs
true
}
} else {
unreachable!("Coding error: unexpected RPCSend variant {rpc_send:?}.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to have this unreachable here? What about a debug assert?

}

// Wait until the tokens have been regenerated, then run `next_peer_request_ready`.
tokio::time::sleep(Duration::from_secs(3)).await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you set a custom quota so we don't have to wait 3 seconds here?

@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Nov 7, 2025
@mergify mergify bot closed this Dec 7, 2025
@mergify
Copy link

mergify bot commented Dec 7, 2025

Hi @ackintosh, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

@mergify mergify bot added the stale Stale PRs that have been inactive and is now outdated label Dec 7, 2025
@ackintosh ackintosh reopened this Jan 12, 2026
@ackintosh ackintosh added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. stale Stale PRs that have been inactive and is now outdated labels Jan 12, 2026
@ackintosh
Copy link
Member Author

@dapplion @jxs Thanks for your feedback! I've updated the PR accordingly.

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks good to me! The test is solid too

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 10, 2026
@mergify mergify bot added the queued label Feb 10, 2026
@mergify
Copy link

mergify bot commented Feb 10, 2026

Merge Queue Status

Rule: default


This pull request spent 30 minutes 58 seconds in the queue, including 29 minutes 3 seconds running CI.

Required conditions to merge
  • check-success=local-testnet-success
  • check-success=test-suite-success

mergify bot added a commit that referenced this pull request Feb 10, 2026
mergify bot added a commit that referenced this pull request Feb 10, 2026
@mergify mergify bot merged commit 889946c into sigp:unstable Feb 10, 2026
36 checks passed
@mergify mergify bot removed the queued label Feb 10, 2026
@ackintosh ackintosh deleted the remove-pending-requests-from-ready-requests branch February 10, 2026 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Networking ready-for-merge This PR is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants