Merge lp:~jc.redoutey/libmemcached/failure_number_fix into lp:~tangent-org/libmemcached/trunk

[JC Redoutey]

[JC Redoutey (jc.redoutey)]

This small patch aims at preventing increasing the retry counter when quitting a memcached server is intentional.

[Brian Aker (brianaker)]

review: Approve

[Brian Aker (brianaker)]

This is causing random test failures once applied to my local Fedora box. Don't you want to clone the memc in the test?

review: Needs Fixing

[lp:~jc.redoutey/libmemcached/failure_number_fix]

601. By Jean-Charles Redoutey <jc@Jayce2009lnx> on 2009-10-12

now cloning memcached_st

[Brian Aker (brianaker)]

So how does this test compare to what we already have? (Which looks to be incomplete, so I suspect this is better).

BTW I will work out an example in the tests which allow you to have memcached_st release all but one server.

[Brian Aker (brianaker)]

I've extracted this test case (it passes and does show different behavior from the current version). Thanks.

Unmerged revisions

601. By Jean-Charles Redoutey <jc@Jayce2009lnx> on 2009-10-12

now cloning memcached_st

=== modified file 'libmemcached/memcached_quit.c'
--- libmemcached/memcached_quit.c	2009-07-18 17:37:40 +0000
+++ libmemcached/memcached_quit.c	2009-10-12 19:59:10 +0000
@@ -2,10 +2,10 @@
 
 /*
   This closes all connections (forces flush of input as well).
-  
-  Maybe add a host specific, or key specific version? 
-  
-  The reason we send "quit" is that in case we have buffered IO, this 
+
+  Maybe add a host specific, or key specific version?
+
+  The reason we send "quit" is that in case we have buffered IO, this
   will force data to be completed.
 */
 
@@ -18,7 +18,7 @@
       memcached_return rc;
       char buffer[MEMCACHED_MAX_BUFFER];
 
-      if (ptr->root->flags & MEM_BINARY_PROTOCOL) 
+      if (ptr->root->flags & MEM_BINARY_PROTOCOL)
       {
         protocol_binary_request_quit request = {.bytes= {0}};
         request.message.header.request.magic = PROTOCOL_BINARY_REQ;
@@ -30,7 +30,7 @@
         rc= memcached_do(ptr, "quit\r\n", 6, 1);
 
       WATCHPOINT_ASSERT(rc == MEMCACHED_SUCCESS || rc == MEMCACHED_FETCH_NOTFINISHED);
-      
+
       /* read until socket is closed, or there is an error
        * closing the socket before all data is read
        * results in server throwing away all data which is
@@ -49,14 +49,14 @@
     memcached_server_response_reset(ptr);
   }
 
-  ptr->server_failure_counter++;
+  if(io_death) ptr->server_failure_counter++;
 }
 
 void memcached_quit(memcached_st *ptr)
 {
   unsigned int x;
 
-  if (ptr->hosts == NULL || 
+  if (ptr->hosts == NULL ||
       ptr->number_of_hosts == 0)
     return;
 

=== modified file 'tests/function.c'
--- tests/function.c	2009-10-12 16:10:17 +0000
+++ tests/function.c	2009-10-12 19:59:10 +0000
@@ -4732,6 +4732,7 @@
   return TEST_SUCCESS;
 }
 
+<<<<<<< TREE
 
 
 /* Test memcached_server_get_last_disconnect
@@ -4786,6 +4787,58 @@
 }
 
 
+=======
+
+/*
+ * This tests ensures expected disconnections (for some behavior changes
+ * for instance) do not wrongly increase failure counter
+ */
+static test_return_t wrong_failure_counter_test(memcached_st *memc)
+{
+  memcached_return rc;
+
+  memcached_st *memc_clone;
+  memc_clone= memcached_clone(NULL, memc);
+  assert(memc_clone);
+
+  /* Set value to force connection to the server */
+  const char *key= "marmotte";
+  const char *value= "milka";
+  char *string = NULL;
+  size_t string_length;
+  uint32_t flags;
+
+  rc= memcached_set(memc_clone, key, strlen(key),
+                    value, strlen(value),
+                    (time_t)0, (uint32_t)0);
+  assert(rc == MEMCACHED_SUCCESS || rc == MEMCACHED_BUFFERED);
+
+
+  /* put failure limit to 1 */
+  rc= memcached_behavior_set(memc_clone, MEMCACHED_BEHAVIOR_SERVER_FAILURE_LIMIT, 1);
+  assert(rc == MEMCACHED_SUCCESS);
+  /* Put a retry timeout to effectively activate failure_limit effect */
+  rc= memcached_behavior_set(memc_clone, MEMCACHED_BEHAVIOR_RETRY_TIMEOUT, 1);
+  assert(rc == MEMCACHED_SUCCESS);
+  /* change behavior that triggers memcached_quit()*/
+  rc= memcached_behavior_set(memc_clone, MEMCACHED_BEHAVIOR_TCP_NODELAY, 1);
+  assert(rc == MEMCACHED_SUCCESS);
+
+
+  /* Check if we still are connected */
+  string= memcached_get(memc_clone, key, strlen(key),
+                        &string_length, &flags, &rc);
+
+  assert(rc == MEMCACHED_SUCCESS);
+  assert(string);
+  free(string);
+
+  return TEST_SUCCESS;
+}
+
+
+
+>>>>>>> MERGE-SOURCE
 test_st udp_setup_server_tests[] ={
   {"set_udp_behavior_test", 0, set_udp_behavior_test},
   {"add_tcp_server_udp_client_test", 0, add_tcp_server_udp_client_test},
@@ -4930,6 +4983,7 @@
   {"user_supplied_bug19", 1, user_supplied_bug19 },
   {"user_supplied_bug20", 1, user_supplied_bug20 },
   {"user_supplied_bug21", 1, user_supplied_bug21 },
+  {"wrong_failure_counter_test", 1, wrong_failure_counter_test},
   {0, 0, 0}
 };