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

pipe reader: don't mark commands as consumed until pending=0 #347

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Conversation

caleblloyd
Copy link
Collaborator

Includes test from #346 (e90d588) - thanks for recreating @mtmk

Previously, commands that were partially trimmed but not sent had marked their data from the pipe reader as Consumed. On a Reconnect, the same amount of data would get Consumed again.

Refactors the Pipe Reader to only mark data as consumed once an entire command has been processed (pending == 0)

Also adds a bunch of comments to help explain the flow

@caleblloyd caleblloyd requested a review from mtmk January 22, 2024 21:50
Copy link
Collaborator

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

LGTM

@mtmk mtmk merged commit f44a527 into main Jan 23, 2024
9 of 10 checks passed
@mtmk mtmk deleted the 341-fix branch January 23, 2024 12:50
@mtmk mtmk mentioned this pull request Jan 23, 2024
mtmk added a commit that referenced this pull request Jan 23, 2024
* Async event handlers (#324)
* pipe reader: don't mark commands as consumed until pending=0 (#347)
* Log connection failure as warning (#343)
@mtmk mtmk mentioned this pull request Jan 23, 2024
mtmk added a commit that referenced this pull request Jan 23, 2024
* Async event handlers (#324)
* pipe reader: don't mark commands as consumed until pending=0 (#347)
* Log connection failure as warning (#343)
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