Bug 394442: Replication mget test fail on linux due to race conditions

All replicas are stored on the different servers by using the quiet commands,
and when we start to receive them we will do that in another memcached_st
instance (aka another connection to the server). The code goes directly
from the SET command to trying to fetch the items from all of the servers.
This means that the memcached server may still be processing the quiet set
commands while another thread in the memcached server tries to fetch all of
the items.

To fix this we need to ensure that all of the set commands are executed on
all of the memcached servers before starting to receive them. memcached_quit
will send the QUIT command to all of the servers and wait for a response,
so we now that when memcached_quit returns all commands are executed on
the client.

In addition the response counter should not be updated when we send out the
replica storage / delete commands.

diff --git a/libmemcached/memcached_delete.c b/libmemcached/memcached_delete.c
index 5e2a0132338aa8a5ce9c9ab394d2ec620a29be21..978ec29c1aaa183a2c2c8d4e8551a526bdafb056 100644 (file)
--- a/libmemcached/memcached_delete.c
+++ b/libmemcached/memcached_delete.c
@@ -143,7 +143,9 @@ static inline memcached_return binary_delete(memcached_st *ptr,
       if ((memcached_do(server, (const char*)request.bytes, 
                         sizeof(request.bytes), 0) != MEMCACHED_SUCCESS) ||
           (memcached_io_write(server, key, key_length, flush) == -1))
-         memcached_io_reset(server);
+        memcached_io_reset(server);
+      else
+        memcached_server_response_decrement(server);
     }
   }
 

diff --git a/libmemcached/memcached_storage.c b/libmemcached/memcached_storage.c
index b950dc92f45e8bebc02a052636533019714987d5..53d79822ae633f5b7eabefd1eea55fc77bac4025 100644 (file)
--- a/libmemcached/memcached_storage.c
+++ b/libmemcached/memcached_storage.c
@@ -491,7 +491,9 @@ static memcached_return memcached_send_binary(memcached_st *ptr,
                         send_length, 0) != MEMCACHED_SUCCESS) ||
           (memcached_io_write(srv, key, key_length, 0) == -1) ||
           (memcached_io_write(srv, value, value_length, flush) == -1))
-        memcached_io_reset(server);
+        memcached_io_reset(srv);
+      else
+        memcached_server_response_decrement(srv);
     }
   }
 

diff --git a/tests/function.c b/tests/function.c
index 46cce5657299384decd5fe3814e0d8187758fcb5..5d0c64b986b819413a8dc92728724772ff3a303e 100644 (file)
--- a/tests/function.c
+++ b/tests/function.c
@@ -3070,6 +3070,16 @@ static memcached_return pre_replication(memcached_st *memc)
   return rc;
 }
 
+static memcached_return pre_replication_noblock(memcached_st *memc)
+{
+  memcached_return rc= MEMCACHED_FAILURE;
+  if (pre_replication(memc) == MEMCACHED_SUCCESS && 
+      pre_nonblock(memc) == MEMCACHED_SUCCESS)
+    rc= MEMCACHED_SUCCESS;
+
+  return rc;
+}
+
 static void my_free(memcached_st *ptr __attribute__((unused)), void *mem)
 {
   free(mem);
@@ -3582,6 +3592,21 @@ static test_return replication_set_test(memcached_st *memc)
   rc= memcached_set(memc, "bubba", 5, "0", 1, 0, 0);
   assert(rc == MEMCACHED_SUCCESS);
 
+  /*
+  ** We are using the quiet commands to store the replicas, so we need
+  ** to ensure that all of them are processed before we can continue.
+  ** In the test we go directly from storing the object to trying to
+  ** receive the object from all of the different servers, so we
+  ** could end up in a race condition (the memcached server hasn't yet
+  ** processed the quiet command from the replication set when it process
+  ** the request from the other client (created by the clone)). As a
+  ** workaround for that we call memcached_quit to send the quit command
+  ** to the server and wait for the response ;-) If you use the test code
+  ** as an example for your own code, please note that you shouldn't need
+  ** to do this ;-)
+  */ 
+  memcached_quit(memc);
+
   /*
   ** "bubba" should now be stored on all of our servers. We don't have an
   ** easy to use API to address each individual server, so I'll just iterate
@@ -3652,6 +3677,21 @@ static test_return replication_mget_test(memcached_st *memc)
     assert(rc == MEMCACHED_SUCCESS);
   }
 
+  /*
+  ** We are using the quiet commands to store the replicas, so we need
+  ** to ensure that all of them are processed before we can continue.
+  ** In the test we go directly from storing the object to trying to
+  ** receive the object from all of the different servers, so we
+  ** could end up in a race condition (the memcached server hasn't yet
+  ** processed the quiet command from the replication set when it process
+  ** the request from the other client (created by the clone)). As a
+  ** workaround for that we call memcached_quit to send the quit command
+  ** to the server and wait for the response ;-) If you use the test code
+  ** as an example for your own code, please note that you shouldn't need
+  ** to do this ;-)
+  */ 
+  memcached_quit(memc);
+
   /*
    * Don't do the following in your code. I am abusing the internal details
    * within the library, and this is not a supported interface.
@@ -4588,6 +4628,7 @@ collection_st collection[] ={
   {"consistent_ketama_weighted", pre_behavior_ketama_weighted, 0, consistent_weighted_tests},
   {"test_hashes", 0, 0, hash_tests},
   {"replication", pre_replication, 0, replication_tests},
+  {"replication_noblock", pre_replication_noblock, 0, replication_tests},
   {0, 0, 0, 0}
 };