Issue 140 in redis: calling setnx on existing key with expire returns 1

[re...@googlecode.com]

Status: New
Owner: ----
Labels: Type-Defect Priority-Medium

New issue 140 by larsburgess: calling setnx on existing key with expire
returns 1
http://code.google.com/p/redis/issues/detail?id=140

What steps will reproduce the problem?

example with expire:
$ ./redis-cli flushdb; ./redis-cli setnx 'abc' 1; ./redis-cli expire 'abc'
10; ./redis-cli setnx 'abc' 2
OK
(integer) 1
(integer) 1
(integer) 1

without expire:
$ ./redis-cli flushdb; ./redis-cli setnx 'abc' 1; ./redis-cli setnx 'abc' 2
OK
(integer) 1
(integer) 0

What is the expected output? What do you see instead?
Expected output should be: 1, 1, 0 since the key exists when setnx is
called the second time.

What version of the product are you using? On what operating system?
1.3.2 version from github, this tree:
http://github.com/antirez/redis/tree/20f5b3886761a0ba963fee435bba90dd09bd5bd5

Please provide any additional information below.
We are trying to use expire key along with setnx to implement a simple lock
as described here:
http://code.google.com/p/redis/wiki/SetnxCommand

This works in redis 1.001:
$ ./redis-cli flushdb; ./redis-cli setnx 'abc' 1; ./redis-cli expire 'abc'
10; ./redis-cli setnx 'abc' 2
OK
(integer) 1
(integer) 1
(integer) 0

Here is some ruby code which also demonstrates the use case (we initialized
redis ruby client with :thread_safe => true):

def get_lock(dockey,timeout=0)
lock_key = _lock_key(dockey)
until @@db.setnx(lock_key,1) do
sleep(1)
end
@@db.expire(lock_key,timeout+1)
Time.now.to_i+timeout+1
end

def release_lock(dockey,lock)
if (lock >= Time.now.to_i)
@@db.del(_lock_key(dockey))
end
end

When get_lock is called, it loops until setnx returns true (meaning either
the key expired or the thread holding the lock has called
release_lock).

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings

[re...@googlecode.com]

Comment #1 on issue 140 by ke...@mailbox.hu: calling setnx on existing key

with expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

I use the version 1.2.1, and I found this setnx/expire/setnx bug.

[Matt Todd]

This sounds like the expected behavior...

When an expiration is set, Redis treats that value as good as gone. The expiration makes it so that the value is accessible for reads until the expiration is over, but any writes means that the value is treated as if it doesn't exist.

For instance, if I'm incrementing a key, expire it, then increment it again, it'll treat it as if I hadn't incremented it the first time.

INCR test

1

INCR test

2

EXPIRE test 10

1

GET test

2

INCR test

1

Does this make sense? Does it align with the behavior you're experiencing?

It would seem that if it didn't behave like this previously, the old version is actually buggy.

--
You received this message because you are subscribed to the Google Groups "Redis DB" group.
To post to this group, send email to redi...@googlegroups.com.
To unsubscribe from this group, send email to redis-db+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/redis-db?hl=en.

--
Matt Todd
Highgroove Studios
www.highgroove.com
cell: 404-314-2612
blog: maraby.org

Scout - Web Monitoring and Reporting Software
www.scoutapp.com

[re...@googlecode.com]

Comment #2 on issue 140 by stkaes: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

The funny thing is that this is by design.

There's an explicit test for this behavior in test-redis.tcl.

IMHO, the design is broken.

SETNX should never succeed if the key exists.

[re...@googlecode.com]

Comment #3 on issue 140 by stkaes: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

OK. here's a patch including new tests.

Attachments:
setnxfix.patch 2.3 KB

[re...@googlecode.com]

Comment #4 on issue 140 by antirez: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

There is no way to fix this as it's not broken. Please read the EXPIRE
manual page to
understand why this works this way.

Basically every write operation against expiring keys starts deleting the
key, because
otherwise the resulting dataset is time dependent.

Imagine to use the Append Log File with your code + your patch. What
happens? The
AOF is read in very short time so all the keys will probably exist even
with expires
set: you'll end with a different dataset reloading the AOF.

Cheers,
Salvatore

[Salvatore Sanfilippo]

On Wed, Jan 27, 2010 at 12:53 AM, Matt Todd <chio...@gmail.com> wrote:
> This sounds like the expected behavior...

Yes, INCR will try to access the key in read-write, so Redis will
remove the key if it has an expire set. Basically the INCR after the
EXPIRE is like acting against a non existing key.

Btw they can't read your message as there is no forwarding between the
Redis Group and the issue tracker unfortunately (it could be entirely
possible to implement such a feature for Google, and very useful
indeed).

--
Salvatore 'antirez' Sanfilippo
http://invece.org

"Once you have something that grows faster than education grows,
you’re always going to get a pop culture.", Alan Kay

[Matt Todd]

Oh, duh. Whoops!

--
You received this message because you are subscribed to the Google Groups "Redis DB" group.
To post to this group, send email to redi...@googlegroups.com.
To unsubscribe from this group, send email to redis-db+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/redis-db?hl=en.

[re...@googlecode.com]

Comment #5 on issue 140 by stkaes: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

Ah. OK. I understand.

I think it would be helpful to add a note to the nx commands manual pages
which says
that in case of an existing timeout on a key, setnx is equivalent to set.

And maybe close this issue.

Thanks,
Stefan

[re...@googlecode.com]

Comment #6 on issue 140 by stkaes: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

Hm. I'm no longer sure the reasoning which leads to the current
implementation is
valid. I think it's more a property of the current implementation. Please
correct me
if I'm wrong.

I assume that currently each command is simply written to the log file for
replay
after a crash/replication to a slave. So a sequence

SET x 10
EXPIREAT x 2232131212
SETNX x 4

could not be replayed without simulating time, which seems impossible.

However, it would be possible to write a different sequence to the log,
depending on
the actual effects of executing the setnx.

If the key existed when the setnx was executed, the log file sequence
should contain
the sequence

SET x 10
EXPIREAT x 2232131212
NOEXPIRE x

(assuming the last command clears the timeout value on x).

If the key did not exist, the sequence to the logfile should be

SET x 10
EXPIREAT x 2232131212
SET x 4

In other words: the logfile should never contain setnx commands at all.

This would make the replay/replication time independent.

What do you think?

[re...@googlecode.com]

Comment #7 on issue 140 by alex.arnell: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

Wouldn't it be simpler to treat SETNX commands almost like a server side
atomic
GETSET. If the key exists nothing is written to the AOF since nothing
happens to the
internal data structures. If the key does not exist SETNX is translated
into a SET
command and written as such to the AOF. This would solve the expiring time
knowledge issue as well. Since if the key expires within redis it won't
exist and will
get written to AOF as SET "key" value, resulting in the EXPIREAT command
being
removed.

IE:

case when key already exists:

SET x 10 // appears in log
SETNX x 20 // does not appear in log since key already exists

case when key does not exist:

SETNX x 20 // appears in log as SET x 20

[re...@googlecode.com]

Comment #8 on issue 140 by stkaes: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

alex,

the current semantics of all write operations to a key is to clear the
timeout value.
Your proposal doesn't respect this semantics.

[re...@googlecode.com]

Comment #9 on issue 140 by alex.arnell: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

SETNX is not always a write operation however. From the command docs:

"SETNX works exactly like SET with the only difference that if the key
already exists no
operation is performed"

So in my opinion SETNX should not even be considered a write operation
unless it is in
fact setting the key.

[re...@googlecode.com]

Comment #10 on issue 140 by stkaes: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

yeah, not doing anything at all is also a possible interpretation. But in
this case,
a get should also not clear the timestamp, I think.

[re...@googlecode.com]

Comment #11 on issue 140 by alex.arnell: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

Exactly, and that is the case. GETSET commands are not treated as write
command
initially.

http://gist.github.com/289104

[re...@googlecode.com]

Updates:
Status: Verified

Comment #12 on issue 140 by antirez: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

The new behavior allows writes against volatile keys, starting from Redis
2.2 (Redis master) this issue is definitely solved.

Cheers,
Salvatore

[re...@googlecode.com]

Comment #13 on issue 140 by larsburgess: calling setnx on existing key with

expire returns 1
http://code.google.com/p/redis/issues/detail?id=140

Thanks, it works perfectly now!