arthurnn / memcached

A Ruby interface to the libmemcached C client
Academic Free License v3.0
432 stars 127 forks source link

Unknown return code #89

Closed tmm1 closed 11 years ago

tmm1 commented 12 years ago

I'm seeing occasional Memcached::Error with strange return code values:

Unknown return code: 10234
Unknown return code: 51194
Unknown return code: 63482
Unknown return code: 34810
Unknown return code: 55290
Unknown return code: 26640
Unknown return code: 41872

AFAIK, these are all invalid values since Memcached::Lib::MEMCACHED_MAXIMUM_RETURN is 40

tmm1 commented 12 years ago

All the exceptions are from https://github.com/evan/memcached/blob/master/lib/memcached/memcached.rb#L504

ruby/1.8/gems/memcached-1.4.2/lib/memcached/memcached.rb:621:in `reraise'
ruby/1.8/gems/memcached-1.4.2/lib/memcached/memcached.rb:598:in `check_return_code'
ruby/1.8/gems/memcached-1.4.2/lib/memcached/memcached.rb:504:in `get_orig'
ruby/1.8/gems/memcached-1.4.2/lib/memcached/rails.rb:81:in `get_multi'
evan commented 12 years ago

Very curious, but I need a reproducible test case to do anything about it.

tmm1 commented 11 years ago

Found another reference at http://stackoverflow.com/questions/13212516/what-does-the-memcached-return-code-44120-mean

It appears this occurs when poll() times out, since memcached_fetch_result never sets a value for memcached_return *error.

This was fixed upstream by detecting the timeout and setting MEMCACHED_NOTFOUND or MEMCACHED_END accordingly.

evan commented 11 years ago

Nice find. Can you backport the fix?

tmm1 commented 11 years ago

I think this should work, but hard to be sure since so much has changed upstream.

diff --git a/ext/libmemcached-0.32/libmemcached/memcached_fetch.c b/ext/libmemcached-0.32/libmemcached/memcached_fetch.c
index d3f0121..f088ff9 100644
--- a/ext/libmemcached-0.32/libmemcached/memcached_fetch.c
+++ b/ext/libmemcached-0.32/libmemcached/memcached_fetch.c
@@ -52,27 +52,54 @@ memcached_result_st *memcached_fetch_result(memcached_st *ptr,
   }

   if (result == NULL)
-    if ((result= memcached_result_create(ptr, NULL)) == NULL)
+    if ((result= memcached_result_create(ptr, NULL)) == NULL) {
+      *error= MEMCACHED_MEMORY_ALLOCATION_FAILURE;
       return NULL;
+    }

+  *error= MEMCACHED_MAXIMUM_RETURN; // We use this to see if we ever go into the loop
   while ((server = memcached_io_get_readable_server(ptr)) != NULL) 
   {
     char buffer[MEMCACHED_DEFAULT_COMMAND_SIZE];
     *error= memcached_response(server, buffer, sizeof(buffer), result);

-    if (*error == MEMCACHED_SUCCESS)
+    if (*error == MEMCACHED_SUCCESS) {
+      result->count++;
       return result;
-    else if (*error == MEMCACHED_END)
+    } else if (*error == MEMCACHED_END)
       memcached_server_response_reset(server);
     else
       break;
   }

+  if (*error == MEMCACHED_NOTFOUND && result->count)
+  {
+    *error= MEMCACHED_END;
+  }
+  else if (*error == MEMCACHED_MAXIMUM_RETURN && result->count)
+  {
+    *error= MEMCACHED_END;
+  }
+  else if (*error == MEMCACHED_MAXIMUM_RETURN) // while() loop was never entered
+  {
+    *error= MEMCACHED_NOTFOUND;
+  }
+  else if (*error == MEMCACHED_SUCCESS)
+  {
+    *error= MEMCACHED_END;
+  }
+  else if (result->count == 0)
+  {
+    *error= MEMCACHED_NOTFOUND;
+  }
+
   /* We have completed reading data */
   if (result->is_allocated)
     memcached_result_free(result);
-  else
+  else {
+    result->count = 0;
     memcached_string_reset(&result->value);
+  }

   return NULL;
 }
diff --git a/ext/libmemcached-0.32/libmemcached/memcached_result.c b/ext/libmemcached-0.32/libmemcached/memcached_result.c
index 0d77130..49545b7 100644
--- a/ext/libmemcached-0.32/libmemcached/memcached_result.c
+++ b/ext/libmemcached-0.32/libmemcached/memcached_result.c
@@ -22,6 +22,7 @@ memcached_result_st *memcached_result_create(memcached_st *memc,
   }

   ptr->root= memc;
+  ptr->count= 0;
   memcached_string_create(memc, &ptr->value, 0);
   WATCHPOINT_ASSERT(ptr->value.string == NULL);

@@ -31,6 +32,7 @@ memcached_result_st *memcached_result_create(memcached_st *memc,
 void memcached_result_reset(memcached_result_st *ptr)
 {
   ptr->key_length= 0;
+  ptr->count= 0;
   memcached_string_reset(&ptr->value);
   ptr->flags= 0;
   ptr->cas= 0;
diff --git a/ext/libmemcached-0.32/libmemcached/memcached_result.h b/ext/libmemcached-0.32/libmemcached/memcached_result.h
index e7ce012..e9b61eb 100644
--- a/ext/libmemcached-0.32/libmemcached/memcached_result.h
+++ b/ext/libmemcached-0.32/libmemcached/memcached_result.h
@@ -21,6 +21,7 @@ struct memcached_result_st {
   size_t key_length;
   uint64_t cas;
   memcached_string_st value;
+  uint64_t count;
   char key[MEMCACHED_MAX_KEY];
   /* Add result callback function */
 };
evan commented 11 years ago

These changes don't consistently pass tests for me. Can you put together a regular pull request with both and make sure the tests all pass?

tmm1 commented 11 years ago

I'm seeing failures as well, stuff like:

<Memcached::ABadKeyWasProvidedOrCharactersOutOfRange> exception expected but was
Class: <Memcached::ATimeoutOccurred>
Message: <"Key {\"i have a space\"=>nil}">

I tried to dig in but can't figure out what's going on.

evan commented 11 years ago

Do you see them on master? Master is clean for me.

tmm1 commented 11 years ago

Master is clean.

My patch is introducing a number of failures. I was able to fix some of them, but I can't figure out why bad keys are causing timeout errors with the patch.

At this point, I think time would be better spent upgrading the libmemcached dependency inside the gem. I took at stab at this, but ran into some issues with the CC settings in extconf since upstream is now C++.

evan commented 11 years ago

If you want to fork and do the upgrade, ok, but I can't upgrade this gem because libmemcached > 0.4 had a bunch of performance regressions that are unlikely to be fixed. See #80, #27.

evan commented 11 years ago

This guy seems to have a fork with an upgraded lib: https://github.com/chetan/memcached/tree/1.0.14

I tried to run it but got segfaults. Maybe you can get it working and do a benchmark comparison.