[memcached] bradfitz, r337: fix some long-standing psuedo-bugs with ...

commits at code.sixapart.com commits at code.sixapart.com
Mon Sep 4 05:29:06 UTC 2006


fix some long-standing psuedo-bugs with regard to the delete-locked
window (when you specify a time parameter to the delete command, which
prevents adds/replaces for that many seconds)

in particular, if you had a small timeout, less than 5 seconds, then a
future add/replace which should've worked wouldn't because the
delete_timer wouldn't have run yet.

this has caused a lot of people just evalutating memcached and playing
around as they read the protocol docs to go crazy, as they couldn't
see the delete timer happening, and it all just appeared random to
them

this also unifies some duplicated code, and adds some assertions, in
particular in assoc_insert to make sure that key isn't already in the
hashtable.



U   trunk/server/assoc.c
U   trunk/server/memcached.c
U   trunk/server/test/delete-window.t


Modified: trunk/server/assoc.c
===================================================================
--- trunk/server/assoc.c	2006-09-04 04:09:41 UTC (rev 336)
+++ trunk/server/assoc.c	2006-09-04 05:29:05 UTC (rev 337)
@@ -164,7 +164,9 @@
 
 /* Note: this isn't an assoc_update.  The key must not already exist to call this */
 int assoc_insert(char *key, item *it) {
-    ub4 hv = hash(key, strlen(key), 0) & hashmask(HASHPOWER);
+    ub4 hv;
+    assert(assoc_find(key) == 0);  /* shouldn't have duplicately named things defined */
+    hv = hash(key, strlen(key), 0) & hashmask(HASHPOWER);
     it->h_next = hashtable[hv];
     hashtable[hv] = it;
     return 1;

Modified: trunk/server/memcached.c
===================================================================
--- trunk/server/memcached.c	2006-09-04 04:09:41 UTC (rev 336)
+++ trunk/server/memcached.c	2006-09-04 05:29:05 UTC (rev 337)
@@ -107,11 +107,26 @@
     settings.factor = 1.25;
     settings.chunk_size = 48;         /* space for a modest key and value */
 }
+
+/* returns true if a deleted item's delete-locked-time is over, and it
+   should be removed from the namespace */
+int item_delete_lock_over (item *it) {
+    assert(it->it_flags & ITEM_DELETED);
+    return (current_time >= it->exptime);
+}
+
 /* wrapper around assoc_find which does the lazy expiration/deletion logic */
-item *get_item(char *key) {
+item *get_item_notedeleted(char *key, int *delete_locked) {
     item *it = assoc_find(key);
+    if (delete_locked) *delete_locked = 0;
     if (it && (it->it_flags & ITEM_DELETED)) {
-        it = 0;
+        /* it's flagged as delete-locked.  let's see if that condition
+           is past due, and the 5-second delete_timer just hasn't
+           gotten to it yet... */
+        if (! item_delete_lock_over(it)) {
+            if (delete_locked) *delete_locked = 1;
+            it = 0;
+        }
     }
     if (it && settings.oldest_live && settings.oldest_live <= current_time &&
         it->time <= settings.oldest_live) {
@@ -125,6 +140,10 @@
     return it;
 }
 
+item *get_item(char *key) {
+    return get_item_notedeleted(key, 0);
+}
+
 /*
  * Adds a message header to a connection.
  *
@@ -557,57 +576,54 @@
     item *it = c->item;
     int comm = c->item_comm;
     item *old_it;
-    rel_time_t now = current_time;
-
+    int delete_locked = 0;
     stats.set_cmds++;
+    char *key = ITEM_key(it);
 
-    while(1) {
-        if (strncmp(ITEM_data(it) + it->nbytes - 2, "\r\n", 2) != 0) {
-            out_string(c, "CLIENT_ERROR bad data chunk");
-            break;
-        }
+    if (strncmp(ITEM_data(it) + it->nbytes - 2, "\r\n", 2) != 0) {
+        out_string(c, "CLIENT_ERROR bad data chunk");
+        goto err;
+    }
 
-        old_it = assoc_find(ITEM_key(it));
+    old_it = get_item_notedeleted(key, &delete_locked);
 
-        if (old_it && settings.oldest_live &&
-            old_it->time <= settings.oldest_live) {
-            item_unlink(old_it);
-            old_it = 0;
-        }
+    if (old_it && comm == NREAD_ADD) {
+        item_update(old_it);  /* touches item, promotes to head of LRU */
+        out_string(c, "NOT_STORED");
+        goto err;
+    }
 
-        if (old_it && old_it->exptime && old_it->exptime < now) {
-            item_unlink(old_it);
-            old_it = 0;
-        }
+    if (!old_it && comm == NREAD_REPLACE) {
+        out_string(c, "NOT_STORED");
+        goto err;
+    }
 
-        if (old_it && comm==NREAD_ADD) {
-            item_update(old_it);
+    if (delete_locked) {
+        if (comm == NREAD_REPLACE || comm == NREAD_ADD) {
             out_string(c, "NOT_STORED");
-            break;
+            goto err;
         }
 
-        if (!old_it && comm == NREAD_REPLACE) {
-            out_string(c, "NOT_STORED");
-            break;
-        }
+        /* but "set" commands can override the delete lock
+         window... in which case we have to find the old hidden item
+         that's in the namespace/LRU but wasn't returned by
+         get_item.... because we need to replace it (below) */
+        old_it = assoc_find(key);
+    }
 
-        if (old_it && (old_it->it_flags & ITEM_DELETED) && (comm == NREAD_REPLACE || comm == NREAD_ADD)) {
-            out_string(c, "NOT_STORED");
-            break;
-        }
+    if (old_it)
+        item_replace(old_it, it);
+    else
+        item_link(it);
 
-        if (old_it) {
-            item_replace(old_it, it);
-        } else item_link(it);
-
-        c->item = 0;
-        out_string(c, "STORED");
-        return;
-    }
-
-    item_free(it);
     c->item = 0;
+    out_string(c, "STORED");
     return;
+
+err:
+     item_free(it);
+     c->item = 0;
+     return;
 }
 
 void process_stat(conn *c, char *command) {
@@ -1827,7 +1843,7 @@
         rel_time_t now = current_time;
         for (i=0; i<delcurr; i++) {
             item *it = todelete[i];
-            if (it->exptime < now) {
+            if (item_delete_lock_over(it)) {
                 assert(it->refcount > 0);
                 it->it_flags &= ~ITEM_DELETED;
                 item_unlink(it);

Modified: trunk/server/test/delete-window.t
===================================================================
--- trunk/server/test/delete-window.t	2006-09-04 04:09:41 UTC (rev 336)
+++ trunk/server/test/delete-window.t	2006-09-04 05:29:05 UTC (rev 337)
@@ -1,7 +1,7 @@
 #!/usr/bin/perl
 
 use strict;
-use Test::More tests => 14;
+use Test::More tests => 20;
 use FindBin qw($Bin);
 use lib "$Bin/lib";
 use MemcachedTest;
@@ -45,13 +45,19 @@
 print $sock "add foo 0 0 7\r\nfoo-add\r\n";
 is($line->(), "STORED\r\n", "stored foo-add");
 
+my $get_fooadd = sub {
+    my $msg = shift;
+    print $sock "get foo\r\n";
+    is($line->(), "VALUE foo 0 7\r\n", "got foo value... ($msg)");
+    is($line->(), "foo-add\r\n", ".. with value foo-add ($msg)");
+    is($line->(), "END\r\n", "got END ($msg)");
+};
+$get_fooadd->("before deleter event");
+
 # wait fo
 diag("waiting 5 seconds for the deleter event...");
 sleep(5.2);
-print $sock "get foo\r\n";
-is($line->(), "VALUE foo 0 7\r\n", "got foo value...");
-is($line->(), "foo-add\r\n", ".. with value foo-add");
-is($line->(), "END\r\n", "got END");
+$get_fooadd->("after deleter event");
 
 
 __END__




More information about the memcached-commits mailing list