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

add stress tests and fix broken pipe when send too fast #124

Merged
merged 2 commits into from
Feb 6, 2025
Merged

Conversation

neonphog
Copy link
Collaborator

@neonphog neonphog commented Feb 5, 2025

RESOLVES: #123

@neonphog neonphog requested a review from a team February 5, 2025 18:19
@neonphog neonphog changed the title add stress tests add stress tests and fix broken pipe when send too fast Feb 5, 2025
Comment on lines +111 to +113
// hard-coded to 1 minute for now. This indicates a system
// is very backed up, and is here just to prevent forever hangs
std::time::Duration::from_secs(60),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lemme know if y'all are okay with this as an outside safeguard, or if it needs to be configurable

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

This begs the question if there's something wrong with the channel, so to say? If the async method does not overflow it but the sync one does.

Is it a problem now that the channel doesn't get closed anymore if sending fails?

@neonphog
Copy link
Collaborator Author

neonphog commented Feb 5, 2025

This begs the question if there's something wrong with the channel, so to say? If the async method does not overflow it but the sync one does.

This is exactly the solution... because the async one will wait until there is space before pushing the new message on.

Is it a problem now that the channel doesn't get closed anymore if sending fails?

The channel does get closed on send failure. There's a match at the end of that function that does it.

@jost-s
Copy link
Contributor

jost-s commented Feb 5, 2025

This is exactly the solution... because the async one will wait until there is space before pushing the new message on.

Okay, that wasn't obvious to me from the docs.

Copy link
Member

@ThetaSinner ThetaSinner left a comment

Choose a reason for hiding this comment

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

Yay for stress tests and this fix looks reasonable to me.

I think having some timeout is reasonable for now. It might give more context to require people making a network request to decide their own timeout but that's an adjustment we can make later if needed. It's good to have something in place for now

@neonphog neonphog merged commit 77356d4 into main Feb 6, 2025
5 of 7 checks passed
@neonphog neonphog deleted the stress branch February 6, 2025 14:49
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.

[bug] sending too fast closes connections
3 participants