Skip to content

Fixes LPOP/RPOP wrong replies when count is 0#9692

Merged
oranagra merged 2 commits intoredis:unstablefrom
itamarhaber:fix-9680
Nov 4, 2021
Merged

Fixes LPOP/RPOP wrong replies when count is 0#9692
oranagra merged 2 commits intoredis:unstablefrom
itamarhaber:fix-9680

Conversation

@itamarhaber
Copy link
Member

@itamarhaber itamarhaber commented Oct 27, 2021 •
edited
Loading

Fixes #9680.

Introduced in #8179, this fixes the command's replies in the 0 count edge case.

  • [BREAKING] changes the reply type when count is 0 to an empty array (instead of nil)
  • Moves LPOP ... 0 fast exit path after type check to reply with WRONGTYPE

TOOD:

  • Update docs with reply type

@itamarhaber
Fixes redis#9680
a1174d1 
* [BREAKING] changes the reply type when count is 0
* Moves LPOP ... 0 fast exit path after type check to reply with WRONGTYPE
@itamarhaber itamarhaber added release-notes indication that this issue needs to be mentioned in the release notes breaking-change This change can potentially break existing application state:needs-doc-pr requires a PR to redis-doc repository labels Oct 27, 2021
zuiderkwast
zuiderkwast approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Very nice. It's a breaking change indeed but it's a bugfix.

This is a typical bug which can be blamed partly on TCL's types (null-string, null-array, empty array and empty string are all represented as an empty TCL string) and partly on the existence of null-arrays all togther...

madolson
madolson approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Lgtm

@itamarhaber
Copy link
Member Author

itamarhaber commented Oct 27, 2021

@zuiderkwast thanks for the kind words, but the RCA points at layer 8.

@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Oct 28, 2021 •
edited
Loading

Another thought, why should we allow count == 0?
The use case count == 0 will always result empty, i don't see any usage scenarios.
In LMPOP/ZMPOP, I made a restriction that count can only be greater than 0 ....
If we already a breaking change, I think we can discuss one more time

The behavior of LMPOP is as follows

// unstable branch
127.0.0.1:6379> lmpop 2 non1 non2 left count 2
(nil)
127.0.0.1:6379> lmpop 2 non1 non2 left count 0
(error) ERR count should be greater than 0

oranagra
oranagra approved these changes Oct 28, 2021
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

I agree with @enjoy-binbin this is not a real use case for this command and we can chose to return an error.
however, i also agree with the fix in this PR, we can also chose to return an empty array.

Nil is certainly a bug though, and since this is a new variant for the command, and a bug in a non-common use case, i think we should also backport it to 6.2.

@oranagra oranagra merged commit 06dd202 into redis:unstable Nov 4, 2021
@enjoy-binbin enjoy-binbin mentioned this pull request Jan 11, 2022
Merged
This was referenced Apr 27, 2022
Closed
Merged
oranagra pushed a commit that referenced this pull request Apr 27, 2022
@itamarhaber @oranagra
Fixes LPOP/RPOP wrong replies when count is 0 (#9692)
968cd2b 
Introduced in #8179, this fixes the command's replies in the 0 count edge case.
[BREAKING] changes the reply type when count is 0 to an empty array (instead of nil)
Moves LPOP ... 0 fast exit path after type check to reply with WRONGTYPE

(cherry picked from commit 06dd202)
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

+3 more reviewers

@madolson madolson madolson approved these changes

@zuiderkwast zuiderkwast zuiderkwast approved these changes

@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:needs-doc-pr requires a PR to redis-doc repository

Projects

Redis 6.2 Backport
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] lpop/pop with count 0 returning null

5 participants

@itamarhaber @enjoy-binbin @zuiderkwast @oranagra @madolson