Skip to content

Fix protocol error caused by redis-benchmark#10236

Merged
oranagra merged 1 commit intoredis:unstablefrom
ivanstosic-janea:benchmark-protocol-error
Feb 7, 2022
Merged

Fix protocol error caused by redis-benchmark#10236
oranagra merged 1 commit intoredis:unstablefrom
ivanstosic-janea:benchmark-protocol-error

Conversation

@ivanstosic-janea
Copy link
Contributor

@ivanstosic-janea ivanstosic-janea commented Feb 4, 2022 •
edited by oranagra
Loading

The protocol error was caused by the buggy writeHandler in redis-benchmark.c,
which didn't handle one of the cases, thereby repeating data, leading to protocol errors
when the values being sent are very long.

This PR fixes #10233, issue introduced by #7959

@oranagra
Copy link
Member

oranagra commented Feb 4, 2022

@ivanstosic-janea I see you submitted a patch for 6.2, is this issue already resolved in unstable (our development branch?
(I'm AFK)

@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Feb 4, 2022 •
edited
Loading

i did not take a deep look, only tested it on the unstable branch (note without TLS), it (src/redis-benchmark -P 1 -t set -n 1 -c 1 -d 50000000) will hang (for a long time). and with this patch, it will work (finish soon). so i suppose you can do a test in unstable branch, and point the PR to the unstabe branch.

btw, may i ask the Protocol error, what exactly does the error look like (hang ?)

@ivanstosic-janea
Copy link
Contributor Author

ivanstosic-janea commented Feb 4, 2022

Yes, redis-benchmark hangs, and yes, the bug also exists in unstable. I'll close this one and open another one aimed at unstable.

@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Feb 4, 2022 •
edited
Loading

@ivanstosic-janea actually you can just EDIT the PR, and point to the unstable branch

@ivanstosic-janea ivanstosic-janea changed the base branch from 6.2 to unstable February 4, 2022 16:46
@ivanstosic-janea
Copy link
Contributor Author

ivanstosic-janea commented Feb 4, 2022

Should be good now

oranagra
oranagra reviewed Feb 4, 2022
View reviewed changes
@oranagra oranagra mentioned this pull request Feb 7, 2022
Closed
@oranagra
Copy link
Member

oranagra commented Feb 7, 2022

triggered partial CI (including TLS): https://github.com/redis/redis/actions/runs/1808767646

@oranagra oranagra merged commit bb87560 into redis:unstable Feb 7, 2022
oranagra pushed a commit that referenced this pull request Apr 27, 2022
@ivanstosic-janea @oranagra
Fix protocol error caused by redis-benchmark (#10236)
0a26869 
The protocol error was caused by the buggy `writeHandler` in `redis-benchmark.c`,
which didn't handle one of the cases, thereby repeating data, leading to protocol errors
when the values being sent are very long.

This PR fixes #10233, issue introduced by #7959

(cherry picked from commit bb87560)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

+2 more reviewers

@yoav-steinberg yoav-steinberg yoav-steinberg left review comments

@enjoy-binbin enjoy-binbin enjoy-binbin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Redis 6.2 Backport
Status: Done
Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Protocol error in redis-benchmark

4 participants

@ivanstosic-janea @oranagra @enjoy-binbin @yoav-steinberg