SETNX does not correctly force key expiry.

[GoogleCodeExporter]

What version of Redis you are using, in what kind of Operating System?

Various, particularly 2.2.7 on Linux 64bit.

What is the problem you are experiencing?

SETNX reports recently expired keys as still present.

What steps will reproduce the problem?

while true:
  SETNX "key" "value";
  EXPIRE "key"; 1
  sleep 2;
  SETNX "key" "value";

This may take two or three minutes before it starts going wrong.

Please provide any additional information below.

I've fixed this by patching dbAdd to try expireIfNecessary before failing. 
(patch attached)

Original issue reported on code.google.com by conrad.i...@gmail.com on 28 May 2011 at 12:25

Attachments:

• setnx.patch

[GoogleCodeExporter]

Thanks for spotting this bug and for the patch. I did a trace of the code paths 
being taken with this piece of code in place, and there is one very subtle 
error that occurs with this patch applied. When a slave node receives SETNX for 
a key that both exists, has an expiry set AND is in fact expired, it doesn't 
delete the key from the DB but rather just returns if it should have been 
expired. This is done because the process of expiry is controlled by the 
master. Not deleting the key while returning non-zero for expireIfNeeded 
results in the new key being duplicated, but not added to the database, 
resulting in a leaked key. Subtle, but very real ;-).

My version of the patch (against 2.2) is located here: 
https://github.com/pietern/redis/tree/2.2-issue566

Thanks again!
Pieter

Original comment by pcnoordh...@gmail.com on 13 Jun 2011 at 7:18

• Changed state: Started

[GoogleCodeExporter]

Thanks Pieter!

Conrad

Original comment by conrad.i...@gmail.com on 13 Jun 2011 at 7:29

[GoogleCodeExporter]

Thanks, fixed with two added lines very local to this and another similar bug 
found in 2.2.

2.4 got a completely different Fix I and Pieter designed. Unstable will get the 
same fix as 2.4, I'm porting it into unstable right now since it is not 
possible to automatically cherry pick that.

I merged Pieter's regression tests that are now passing.

Salvatore

Original comment by anti...@gmail.com on 14 Jun 2011 at 3:26

• Changed state: Verified