Skip to content

Fix bug in writeToClient during refactoring I/O threading#834

Merged
madolson merged 1 commit into
valkey-io:unstablefrom
uriyage:fix_write_to_client
Jul 31, 2024
Merged

Fix bug in writeToClient during refactoring I/O threading#834
madolson merged 1 commit into
valkey-io:unstablefrom
uriyage:fix_write_to_client

Conversation

@uriyage

@uriyage uriyage commented Jul 28, 2024

Copy link
Copy Markdown
Contributor

Fix bug in writeToClient
In #758, a major refactor was done to networking.c.

As part of this refactor, a new bug was introduced: we don't advance the c->buf pointer in repeated writes.

This bug should be very unlikely to manifest, as it requires the client's TCP buffer to be filled in the first try and then released immediately after in the second try.

Despite all my efforts to reproduce this scenario, I was unable to do so.

@ranshid ranshid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@madolson

Copy link
Copy Markdown
Member

@uriyage Please fix the DCO.

@madolson madolson added the pending-missing-dco For PRs that are blocked because they are missing a dco label Jul 28, 2024
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
@uriyage uriyage force-pushed the fix_write_to_client branch from f439cbc to 25d0482 Compare July 28, 2024 20:10
@codecov

codecov Bot commented Jul 28, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.38%. Comparing base (e32518d) to head (25d0482).
Report is 2 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #834      +/-   ##
============================================
+ Coverage     70.20%   70.38%   +0.17%     
============================================
  Files           112      112              
  Lines         61462    61462              
============================================
+ Hits          43152    43262     +110     
+ Misses        18310    18200     -110     
Files Coverage Δ
src/networking.c 88.80% <100.00%> (+0.08%) ⬆️

... and 13 files with indirect coverage changes

@madolson madolson merged commit 1d18842 into valkey-io:unstable Jul 31, 2024
@madolson madolson changed the title Fix bug in writeToClient Fix bug in writeToClient during refactoring I/O threading Jul 31, 2024
mapleFU pushed a commit to mapleFU/valkey that referenced this pull request Aug 22, 2024
Fix bug in writeToClient
In valkey-io#758, a major refactor was
done to `networking.c`.

As part of this refactor, a new bug was introduced: we don't advance the
`c->buf` pointer in repeated writes.

This bug should be very unlikely to manifest, as it requires the
client's TCP buffer to be filled in the first try and then released
immediately after in the second try.

Despite all my efforts to reproduce this scenario, I was unable to do
so.

Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Signed-off-by: mwish <maplewish117@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pending-missing-dco For PRs that are blocked because they are missing a dco

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants