Blocking functions: Check key type first, and expire empty keys.#1130
Blocking functions: Check key type first, and expire empty keys.#1130prvyk wants to merge 3 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Thanks for raising this issue!
What Redis does is actually a bit more nuanced here. Let me describe BLPOP for instance - initially, it checks the keys in the order that they are given, if a key has a List value and has an item - that item gets popped. If a value is not a List and is not none - it returns an error.
So essentially, if I have a list of keys k1 (list), k2 (non-list), k3 (list) - it will only fail if k1 is empty.
For example:

Another behavioral detail to keep in mind, if we block on an empty item that gets set to a different type, the operation will continue to wait and will only be released if the type changed to the correct type and an item was found:

That being said, what I think the solution should be -
That initial lookup loop for existing items happens in CollectionItemBroker.InitializeObserver. So, we need to implement a logic where if TryGetResult indicates that the value in key is of the wrong type and return an error accordingly.
Let me know if that makes sense & if you have any follow-up questions.
bc24cb9 to
1c0da12
Compare
It makes perfect sense. I considered that implementation strategy, but wanted to avoid touching the event code, and didn't notice the (valid object) (invalid object) behaviour, so went for the simpler initial GetKeyType()... But this doesn't leave much choice but to change the event code. |
1add783 to
cda2a3a
Compare
4a5798c to
572e1d0
Compare
e29b905 to
b3a8333
Compare
Add more timeout checks. Add test.
… like non-blocking pops do.
|
Obsoleted by #1205 |
Whenever Redis gets a blocking pop command, it tries to pop the value first, and if the value is of the wrong type, it aborts with a WRONGTYPE error. Note that setting the key with the wrong type afterwards (while the timeout is on), will not emit a WRONGTYPE error.
This is arguably in 'bug compatible' category - but blocking when the client may not expect it is best avoided.