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

[catloop] Warn Backlog Length Listen #955

Merged
merged 1 commit into from
Oct 5, 2023
Merged

[catloop] Warn Backlog Length Listen #955

merged 1 commit into from
Oct 5, 2023

Conversation

ppenna
Copy link
Contributor

@ppenna ppenna commented Oct 5, 2023

Description

This PR closes #697.

We currently ignore the backlog length and this PR improves existing code by keeping track of pending connections and triggering logging if a passive socket is too busy.

Ideally we would send ECONNREFUSED back to clients, but the current connection handshake protocol does not support that. It will keep queueing requests anyways.

@ppenna ppenna added the bug Something Isn't Working label Oct 5, 2023
@ppenna ppenna requested a review from iyzhang October 5, 2023 09:48
@ppenna ppenna self-assigned this Oct 5, 2023
@ppenna
Copy link
Contributor Author

ppenna commented Oct 5, 2023

@iyzhang you may keep this issue in mind if you were to redesign the connection establishment protocol.

Copy link
Contributor

@iyzhang iyzhang left a comment

Choose a reason for hiding this comment

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

LGTM

src/rust/catloop/socket.rs Show resolved Hide resolved
src/rust/catloop/socket.rs Show resolved Hide resolved
Copy link
Contributor

@iyzhang iyzhang left a comment

Choose a reason for hiding this comment

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

Bumping the pending number on accept is not correct because the server is going to dequeue that connection request, not enqueue it, so the pending number does not currently reflect the backlog. In order to reflect the backlog, we would have to increase the pending number when the request arrives in the queue.

Copy link
Contributor

@iyzhang iyzhang left a comment

Choose a reason for hiding this comment

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

Thanks for explaining the backlog length to me!

@ppenna ppenna merged commit fda1c8d into dev Oct 5, 2023
11 checks passed
@ppenna ppenna deleted the bugfix-catloop-listen branch October 5, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something Isn't Working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[catloop] Backlog Length Is Completely Ignored
2 participants