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

Fix auto pong responses not flushing after block #393

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

alexheretic
Copy link
Contributor

@alexheretic alexheretic commented Dec 2, 2023

Currently auto pongs are written and flushed during read(). If a WouldBlock error is received it is ignored and reading continues.

In the case the usage is read-predominant, this presents a scenario where the auto pong is WouldBlock-ed and will never retry flushing until another ping is received or the client does some unrelated writes. This could cause the other connected party to time out the connection as it has failed to receive a timely pong response.

I reproduced this in new test read_usage_auto_pong_flush.

To fix we record if a pong write failed in this way and retry on all subsequent read calls until a flush succeeds.

Copy link
Member

@daniel-abramov daniel-abramov left a comment

Choose a reason for hiding this comment

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

Good catch! Thanks!

NB I've now [again] realized that we need to rewrite our state machine (the code is getting increasingly convoluted and hard to manage) (edit: not the fault of this PR, just a general remark).

@daniel-abramov daniel-abramov merged commit 9f0af2a into snapview:master Dec 5, 2023
5 checks passed
@alexheretic alexheretic deleted the fix-auto-pong-flush branch December 12, 2023 12:26
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.

2 participants