From 788f592f4a93a8f4f4ff2721f119f7de269e22c9 Mon Sep 17 00:00:00 2001
From: dormando <dormando@rydia.net>
Date: Fri, 24 May 2024 13:18:48 -0700
Subject: [PATCH] crawler: don't block during hash expansion

if using `lru_crawler metadump hash` the client could block while trying
to grab the maintenance lock potentially held by the hash table
expander. Hash expansion can potentially take a long time.

Further, while waiting for this lock, the crawler is holding the lru
crawler lock, which could cause other clients trying to talk to the
crawler code to themselves block.

So... throw a locked error and don't do that.
---
 assoc.c     |  7 +++++--
 crawler.c   | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
 memcached.h |  1 +
 3 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/assoc.c b/assoc.c
index 8cee570e9e..dbaec45940 100644
--- a/assoc.c
+++ b/assoc.c
@@ -307,8 +307,11 @@ void *assoc_get_iterator(void) {
         return NULL;
     }
     // this will hang the caller while a hash table expansion is running.
-    mutex_lock(&maintenance_lock);
-    return iter;
+    if (mutex_trylock(&maintenance_lock) == 0) {
+        return iter;
+    } else {
+        return NULL;
+    }
 }
 
 bool assoc_iterate(void *iterp, item **it) {
diff --git a/crawler.c b/crawler.c
index 5122054028..16a41586d9 100644
--- a/crawler.c
+++ b/crawler.c
@@ -55,6 +55,7 @@ struct _crawler_module_t {
     void *data; /* opaque data pointer */
     crawler_client_t c;
     crawler_module_reg_t *mod;
+    int status; /* flags/code/etc for internal module usage */
 };
 
 static int crawler_expired_init(crawler_module_t *cm, void *data);
@@ -68,31 +69,33 @@ crawler_module_reg_t crawler_expired_mod = {
     .doneclass = crawler_expired_doneclass,
     .finalize = crawler_expired_finalize,
     .needs_lock = true,
-    .needs_client = false
+    .needs_client = false,
 };
 
+static int crawler_metadump_init(crawler_module_t *cm, void *data);
 static void crawler_metadump_eval(crawler_module_t *cm, item *search, uint32_t hv, int i);
 static void crawler_metadump_finalize(crawler_module_t *cm);
 
 crawler_module_reg_t crawler_metadump_mod = {
-    .init = NULL,
+    .init = crawler_metadump_init,
     .eval = crawler_metadump_eval,
     .doneclass = NULL,
     .finalize = crawler_metadump_finalize,
     .needs_lock = false,
-    .needs_client = true
+    .needs_client = true,
 };
 
+static int crawler_mgdump_init(crawler_module_t *cm, void *data);
 static void crawler_mgdump_eval(crawler_module_t *cm, item *search, uint32_t hv, int i);
 static void crawler_mgdump_finalize(crawler_module_t *cm);
 
 crawler_module_reg_t crawler_mgdump_mod = {
-    .init = NULL,
+    .init = crawler_mgdump_init,
     .eval = crawler_mgdump_eval,
     .doneclass = NULL,
     .finalize = crawler_mgdump_finalize,
     .needs_lock = false,
-    .needs_client = true
+    .needs_client = true,
 };
 
 crawler_module_reg_t *crawler_mod_regs[4] = {
@@ -259,6 +262,11 @@ static void crawler_expired_eval(crawler_module_t *cm, item *search, uint32_t hv
     pthread_mutex_unlock(&d->lock);
 }
 
+static int crawler_metadump_init(crawler_module_t *cm, void *data) {
+    cm->status = 0;
+    return 0;
+}
+
 static void crawler_metadump_eval(crawler_module_t *cm, item *it, uint32_t hv, int i) {
     char keybuf[KEY_MAX_URI_ENCODED_LENGTH];
     int is_flushed = item_is_flushed(it);
@@ -296,12 +304,25 @@ static void crawler_metadump_finalize(crawler_module_t *cm) {
     if (cm->c.c != NULL) {
         // flush any pending data.
         if (lru_crawler_write(&cm->c) == 0) {
-            memcpy(cm->c.buf, "END\r\n", 5);
-            cm->c.bufused += 5;
+            // Only nonzero status right now means we were locked
+            if (cm->status != 0) {
+                const char *errstr = "ERROR locked try again later\r\n";
+                size_t errlen = strlen(errstr);
+                memcpy(cm->c.buf, errstr, errlen);
+                cm->c.bufused += errlen;
+            } else {
+                memcpy(cm->c.buf, "END\r\n", 5);
+                cm->c.bufused += 5;
+            }
         }
     }
 }
 
+static int crawler_mgdump_init(crawler_module_t *cm, void *data) {
+    cm->status = 0;
+    return 0;
+}
+
 static void crawler_mgdump_eval(crawler_module_t *cm, item *it, uint32_t hv, int i) {
     int is_flushed = item_is_flushed(it);
     /* Ignore expired content. */
@@ -335,8 +356,16 @@ static void crawler_mgdump_finalize(crawler_module_t *cm) {
     if (cm->c.c != NULL) {
         // flush any pending data.
         if (lru_crawler_write(&cm->c) == 0) {
-            memcpy(cm->c.buf, "EN\r\n", 4);
-            cm->c.bufused += 4;
+            // Only nonzero status right now means we were locked
+            if (cm->status != 0) {
+                const char *errstr = "ERROR locked try again later\r\n";
+                size_t errlen = strlen(errstr);
+                memcpy(cm->c.buf, errstr, errlen);
+                cm->c.bufused += errlen;
+            } else {
+                memcpy(cm->c.buf, "EN\r\n", 4);
+                cm->c.bufused += 4;
+            }
         }
     }
 }
@@ -420,6 +449,12 @@ static void item_crawl_hash(void) {
     item *it = NULL;
     int items = 0;
 
+    // Could not get the iterator: probably locked due to hash expansion.
+    if (iter == NULL) {
+        active_crawler_mod.status = 1;
+        return;
+    }
+
     // loop while iterator returns something
     // - iterator func handles bucket-walking
     // - iterator returns with bucket locked.
diff --git a/memcached.h b/memcached.h
index 5cf61c18a6..45ca2f36dc 100644
--- a/memcached.h
+++ b/memcached.h
@@ -967,6 +967,7 @@ void conn_worker_readd(conn *c);
 extern int daemonize(int nochdir, int noclose);
 
 #define mutex_lock(x) pthread_mutex_lock(x)
+#define mutex_trylock(x) pthread_mutex_trylock(x)
 #define mutex_unlock(x) pthread_mutex_unlock(x)
 
 #include "stats_prefix.h"