force pushed <https://jausoft.com/cgit/cgit.git/commit/?id=c701a724807d980b7876c3a1c15ea747088ea4c7>
Refined locking errors w/ close_lock. using enum type, removed redundant logging. +++ From c701a724807d980b7876c3a1c15ea747088ea4c7 Mon Sep 17 00:00:00 2001 From: Sven Göthel <[email protected]> Date: Sun, 19 Jul 2026 06:17:56 +0200 Subject: Resolve concurrent initial cache fill post lock. Release lock after fill. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enhance verbose cache_log logging unlock_slot - add: unlock the cache-lock file's lock - this frees the lock right after successfull initial fill and allows concurrent usage lock_slot - lock: add retry on EAGAIN, waiting for concurrent process - uses `cache_lock_timeout` (default 10s) - post-lock: test if cache slot became valid from concurrent process - if successful, unlock_lock and proceed w/ cached_fd - otherwise close_slot (cached_fd) for new cache-slot - failure on truncate and write issues unlock_slot process_slot - remove complicated is_expired branch, simply utilize concurrent check in lock_slot. - If slot doesn't exist or is_expired, close_slot and lock_slot. If the latter produced a cached_fd, use it. Otherwise fill_slot and swap lock-file as new cache-file. Signed-off-by: Sven Göthel <[email protected]> diff --git a/cache.c b/cache.c index 28e7180..3f93aa9 100644 --- a/cache.c +++ b/cache.c @@ -16,6 +16,10 @@ #include "cgit.h" #include "cache.h" #include "html.h" +#include <stdio.h> +#include <fcntl.h> +#include <time.h> +#include <unistd.h> #ifdef HAVE_LINUX_SENDFILE #include <sys/sendfile.h> #endif @@ -76,8 +80,7 @@ static int close_slot(struct cache_slot *slot) if (slot->cache_fd > 0) { if (close(slot->cache_fd)) err = errno; - else - slot->cache_fd = -1; + slot->cache_fd = -1; } return err; } @@ -86,50 +89,52 @@ static int close_slot(struct cache_slot *slot) #define MY_MAX(X, Y) (((X) > (Y)) ? (X) : (Y)) static int sendslot_to_idle(time_t tStart, time_t tLastSend, time_t tNow, - size_t off, size_t size, const char *cache_name) + size_t off, size_t size, struct cache_slot *slot) { const time_t td_total = tNow - tStart; const time_t td_idle = tNow - tLastSend; const long rate = off / MY_MAX(1, td_total); cache_log("[cgit] send_slot timeout idle %lds: sending cache " - "%s (%ld/%ld bytes) to client `%s` " + "%s (%s) (%ld/%ld bytes) to client `%s` " "within [total %lds, idle %lds, rate %ld Bps]\n", - td_idle, cache_name, off, size, ctx.env.remote_addr, + td_idle, slot->cache_name, slot->key, off, size, ctx.env.remote_addr, td_total, td_idle, rate); return ETIMEDOUT; } static int sendslot_to_minrate(time_t tStart, time_t tNow, size_t off, - size_t size, const char *cache_name) + size_t size, struct cache_slot *slot) { const time_t td_total = tNow - tStart; const long rate = off / MY_MAX(1, td_total); cache_log("[cgit] send_slot timeout rate-limit %ld Bps: sending " - "cache %s (%ld/%ld bytes) to client `%s` " + "cache %s (%s) (%ld/%ld bytes) to client `%s` " "within [total %lds, rate %ld Bps]\n", - ctx.cfg.client_io_min_rate, cache_name, off, size, ctx.env.remote_addr, + ctx.cfg.client_io_min_rate, slot->cache_name, slot->key, + off, size, ctx.env.remote_addr, td_total, rate); return ETIMEDOUT; } static int sendslot_ok(time_t tStart, time_t tNow, size_t size, - const char *cache_name) + struct cache_slot *slot) { if (ctx.cfg.log_level > 90) { const time_t td_total = tNow - tStart; const long rate = size / MY_MAX(1, td_total); - cache_log("[cgit] send_slot status: sent cache %s (%ld bytes) to " + cache_log("[cgit] send_slot status: sent cache %s (%s) %ld bytes) to " "client `%s` " "within [total %lds, rate %ld Bps]\n", - cache_name, size, ctx.env.remote_addr, td_total, rate); + slot->cache_name, slot->key, + size, ctx.env.remote_addr, td_total, rate); } return 0; } -static int sendslot_ok2(time_t tStart, size_t size, const char *cache_name) +static int sendslot_ok2(time_t tStart, size_t size, struct cache_slot *slot) { if (ctx.cfg.log_level > 90) { - return sendslot_ok(tStart, time(NULL), size, cache_name); + return sendslot_ok(tStart, time(NULL), size, slot); } return 0; } @@ -192,7 +197,7 @@ static int print_slot(struct cache_slot *slot) off_t size = slot->cache_st.st_size; if (!size) { - return sendslot_ok(tStart, tNow, size, slot->cache_name); + return sendslot_ok(tStart, tNow, size, slot); } const time_t to_min_rate = MY_MAX(ctx.cfg.client_io_idle_timeout, size / ctx.cfg.client_io_min_rate); @@ -200,11 +205,9 @@ static int print_slot(struct cache_slot *slot) #ifdef HAVE_LINUX_SENDFILE do { if (tNow - tLastSend >= ctx.cfg.client_io_idle_timeout) - return sendslot_to_idle(tStart, tLastSend, tNow, - off, size, slot->cache_name); + return sendslot_to_idle(tStart, tLastSend, tNow, off, size, slot); if (tNow - tStart > to_min_rate) - return sendslot_to_minrate(tStart, tNow, - off, size, slot->cache_name); + return sendslot_to_minrate(tStart, tNow, off, size, slot); ssize_t count = sendfile(STDOUT_FILENO, slot->cache_fd, &off, size - off); @@ -219,7 +222,7 @@ static int print_slot(struct cache_slot *slot) } else if (count > 0) { tLastSend = tNow; if (off == size) - return sendslot_ok(tStart, tNow, size, slot->cache_name); + return sendslot_ok(tStart, tNow, size, slot); } } while (1); #endif @@ -239,15 +242,14 @@ static int print_slot(struct cache_slot *slot) if (ETIMEDOUT == errno) { if (-2 == res) return sendslot_to_idle(tStart, tLastSend, time(NULL), - off, size, slot->cache_name); + off, size, slot); else if (-3 == res) - return sendslot_to_minrate(tStart, time(NULL), - off, size, slot->cache_name); + return sendslot_to_minrate(tStart, time(NULL), off, size, slot); } return errno; } if (off == size || !count /* should be redundant */) - return sendslot_ok2(tStart, size, slot->cache_name); + return sendslot_ok2(tStart, size, slot); } while (1); } @@ -260,20 +262,6 @@ static int is_expired(struct cache_slot *slot) return slot->cache_st.st_mtime + slot->ttl * 60 < time(NULL); } -/* Check if the slot has been modified since we opened it. - * NB: If stat() fails, we pretend the file is modified. - */ -static int is_modified(struct cache_slot *slot) -{ - struct stat st; - - if (stat(slot->cache_name, &st)) - return 1; - return (st.st_ino != slot->cache_st.st_ino || - st.st_mtime != slot->cache_st.st_mtime || - st.st_size != slot->cache_st.st_size); -} - /* Close an open lockfile */ static int close_lock(struct cache_slot *slot) { @@ -281,17 +269,28 @@ static int close_lock(struct cache_slot *slot) if (slot->lock_fd > 0) { if (close(slot->lock_fd)) err = errno; - else - slot->lock_fd = -1; + slot->lock_fd = -1; } return err; } +enum lock_file_op_t { UNLINK_LOCK_FILE=0, REPLACE_OLD_SLOT=1 }; + +static int unlock_slot(struct cache_slot *slot, enum lock_file_op_t lock_file_op); + +static const char *to_string(enum lock_file_op_t lock_file_op) { + switch (lock_file_op) { + case UNLINK_LOCK_FILE: return "unlink"; + case REPLACE_OLD_SLOT: return "replace"; + default: return "undef"; + } +} + /* Create a lockfile used to store the generated content for a cache * slot, and write the slot key + \0 into it. * Returns 0 on success and errno otherwise. */ -static int lock_slot(struct cache_slot *slot) +static int lock_slot(struct cache_slot *slot, time_t tStart) { struct flock lock = { .l_type = F_WRLCK, @@ -299,48 +298,129 @@ static int lock_slot(struct cache_slot *slot) .l_start = 0, .l_len = 0, }; + size_t wait_count = 0; slot->lock_fd = open(slot->lock_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); if (slot->lock_fd == -1) return errno; - if (fcntl(slot->lock_fd, F_SETLK, &lock) < 0) { + while (fcntl(slot->lock_fd, F_SETLK, &lock) < 0) { int saved_errno = errno; - close(slot->lock_fd); - slot->lock_fd = -1; + time_t tNow = time(NULL); + if (EAGAIN != saved_errno || + tNow - tStart >= ctx.cfg.cache_lock_timeout) { + close_lock(slot); + cache_log("[cgit] Lock (%ds): Unable to lock slot %s (%s): %s (%d)\n", + (int)(tNow - tStart), slot->lock_name, + slot->key, strerror(saved_errno), saved_errno); + return saved_errno; + } + ++wait_count; + usleep(100000); // 100ms sleep instead of sched_yield() + } + if (wait_count && ctx.cfg.log_level > 90) { + cache_log("[cgit] Lock: Waited %ds (%zu tries, cache_fd %d) to lock slot %s (%s)\n", + (int)(time(NULL) - tStart), wait_count, slot->cache_fd, + slot->lock_name, slot->key); + } + if (slot->cache_fd <= 0) { + int err = open_slot(slot); + if (!err && slot->match) { + // concurrent process wrote the file + if (ctx.cfg.log_level > 50) { + cache_log("[cgit] Lock: Concurrent produced slot %s (%s)\n", + slot->lock_name, slot->key); + } + unlock_slot(slot, UNLINK_LOCK_FILE); + close_lock(slot); + return 0; + } + close_slot(slot); + } + if (ftruncate(slot->lock_fd, 0) < 0) { + int saved_errno = errno; + cache_log("[cgit] Lock (%ds): Unable to truncate locked slot %s (%s): %s (%d)\n", + (int)(time(NULL) - tStart), slot->lock_name, + slot->key, strerror(saved_errno), saved_errno); + unlock_slot(slot, UNLINK_LOCK_FILE); + close_lock(slot); return saved_errno; } - if (ftruncate(slot->lock_fd, 0) < 0) - return errno; - if (xwrite(slot->lock_fd, slot->key, slot->keylen + 1) < 0) - return errno; + if (xwrite(slot->lock_fd, slot->key, slot->keylen + 1) < 0) { + int saved_errno = errno; + cache_log("[cgit] Lock (%ds): Unable to write to locked slot %s (%s): %s (%d)\n", + (int)(time(NULL) - tStart), slot->lock_name, + slot->key, strerror(saved_errno), saved_errno); + unlock_slot(slot, UNLINK_LOCK_FILE); + close_lock(slot); + return saved_errno; + } + if (ctx.cfg.log_level > 90) { + cache_log("[cgit] Lock (%ds): Successful locked slot %s (%s)\n", + (int)(time(NULL) - tStart), slot->lock_name, slot->key); + } return 0; } /* Release the current lockfile. If `replace_old_slot` is set the * lockfile replaces the old cache slot, otherwise the lockfile is * just deleted. + * @param lock_file_op UNLINK_LOCK_FILE unlink or UNLINK_LOCK_FILE replace old-slot w/ lock-file */ -static int unlock_slot(struct cache_slot *slot, int replace_old_slot) +static int unlock_slot(struct cache_slot *slot, enum lock_file_op_t lock_file_op) { - int err; - - if (replace_old_slot) - err = rename(slot->lock_name, slot->cache_name); - else - err = unlink(slot->lock_name); + struct flock lock = { + .l_type = F_UNLCK, + .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 0, + }; + int err = 0; + if (REPLACE_OLD_SLOT == lock_file_op) { + if (rename(slot->lock_name, slot->cache_name)) { + err = errno; + } + } else if (UNLINK_LOCK_FILE == lock_file_op) { + if (unlink(slot->lock_name)) { + err = errno; + } + } + if (ctx.cfg.log_level < 90 && ENOENT == err) { + err = 0; // suppress ENOENT messages + } + if (err) { + cache_log("[cgit] Unlock: Failed to %s slot lock %s, cache %s, key %s: %s (%d)\n", + to_string(lock_file_op), + slot->lock_name, slot->cache_name, slot->key, strerror(err), err); + } + if (ENOENT == err) { // not an error + err = 0; + } /* Restore stdout and close the temporary FD. */ if (slot->stdout_fd >= 0) { dup2(slot->stdout_fd, STDOUT_FILENO); close(slot->stdout_fd); slot->stdout_fd = -1; } - - if (err) - return errno; - - return 0; + if (slot->lock_fd > 0) { + if (fcntl(slot->lock_fd, F_SETLK, &lock) < 0) { + int saved_errno = errno; + close(slot->lock_fd); + slot->lock_fd = -1; + cache_log("[cgit] Unlock: Unable to unlock slot %s (%s): %s (%d)\n", + slot->lock_name, slot->key, strerror(saved_errno), saved_errno); + if (!err) + err = saved_errno; + } + } + if (!err) { + if (ctx.cfg.log_level > 90) { + cache_log("[cgit] Unlock: Successful unlocked slot %s (%s)\n", + slot->lock_name, slot->key); + } + } + return err; } /* Generate the content for the current cache slot by redirecting @@ -396,42 +476,13 @@ unsigned long hash_str(const char *str) static int process_slot(struct cache_slot *slot) { int err; + time_t tStart = time(NULL); err = open_slot(slot); - if (!err && slot->match) { - if (is_expired(slot)) { - if (!lock_slot(slot)) { - /* If the cachefile has been replaced between - * `open_slot` and `lock_slot`, we'll just - * serve the stale content from the original - * cachefile. This way we avoid pruning the - * newly generated slot. The same code-path - * is chosen if fill_slot() fails for some - * reason. - * - * TODO? check if the new slot contains the - * same key as the old one, since we would - * prefer to serve the newest content. - * This will require us to open yet another - * file-descriptor and read and compare the - * key from the new file, so for now we're - * lazy and just ignore the new file. - */ - if (is_modified(slot) || fill_slot(slot)) { - unlock_slot(slot, 0); - close_lock(slot); - } else { - close_slot(slot); - unlock_slot(slot, 1); - slot->cache_fd = slot->lock_fd; - } - } - } - if ((err = print_slot(slot)) != 0) { - cache_log("[cgit] error printing cache %s: %s (%d)\n", - slot->cache_name, - strerror(err), - err); + if (!err && slot->match && !is_expired(slot)) { + if ((err = print_slot(slot)) != 0 && err != ETIMEDOUT) { + cache_log("[cgit] error printing cache %s (%s): %s (%d)\n", + slot->cache_name, slot->key, strerror(err), err); } close_slot(slot); return err; @@ -442,38 +493,42 @@ static int process_slot(struct cache_slot *slot) * request. If this fails (for whatever reason), lets just generate * the content without caching it and fool the caller to believe * everything worked out (but print a warning on stdout). + * + * If the cachefile has been created between + * above `open_slot` and within `lock_slot`, we'll just + * serve the new content from the new cachefile. */ close_slot(slot); - if ((err = lock_slot(slot)) != 0) { - cache_log("[cgit] Unable to lock slot %s: %s (%d)\n", - slot->lock_name, strerror(err), err); + if ((err = lock_slot(slot, tStart)) != 0) { + time_t tNow1 = time(NULL); slot->fn(); return 0; } - - if ((err = fill_slot(slot)) != 0) { - cache_log("[cgit] Unable to fill slot %s: %s (%d)\n", - slot->lock_name, strerror(err), err); - unlock_slot(slot, 0); - close_lock(slot); - slot->fn(); - return 0; - } - // We've got a valid cache slot in the lock file, which - // is about to replace the old cache slot. But if we - // release the lockfile and then try to open the new cache - // slot, we might get a race condition with a concurrent - // writer for the same cache slot (with a different key). - // Lets avoid such a race by just printing the content of - // the lock file. - slot->cache_fd = slot->lock_fd; - unlock_slot(slot, 1); - if ((err = print_slot(slot)) != 0) { - cache_log("[cgit] error printing cache %s: %s (%d)\n", - slot->cache_name, - strerror(err), - err); + if (slot->cache_fd <= 0) { + // first concurrent process lock + if ((err = fill_slot(slot)) != 0) { + cache_log("[cgit] Unable to fill slot %s (%s): %s (%d)\n", + slot->lock_name, slot->key, strerror(err), err); + unlock_slot(slot, UNLINK_LOCK_FILE); + close_lock(slot); + slot->fn(); + return 0; + } + // We've got a valid cache slot in the lock file, which + // is about to replace the old cache slot. But if we + // release the lockfile and then try to open the new cache + // slot, we might get a race condition with a concurrent + // writer for the same cache slot (with a different key). + // Lets avoid such a race by just printing the content of + // the lock file. + slot->cache_fd = slot->lock_fd; + unlock_slot(slot, REPLACE_OLD_SLOT); + } // else concurrent process produced slot (opened) + if ((err = print_slot(slot)) != 0 && err != ETIMEDOUT) { + cache_log("[cgit] error printing cache %s (%s): %s (%d)\n", + slot->cache_name, slot->key, + strerror(err), err); } close_slot(slot); return err; @@ -592,9 +647,22 @@ int cache_ls(const char *path) /* Print a message to stdout */ void cache_log(const char *format, ...) { + char buffer[400]; + char *end = buffer + sizeof(buffer); + char *out = buffer; + *(end - 1) = 0; + struct tm tNowLocal; + time_t tNow = time(NULL); + struct tm *tres = localtime_r(&tNow, &tNowLocal); + if (tres == &tNowLocal) { + // 'YYYY-mm-dd hh:mm:ss ' + out += strftime(out, 20 + 1, "%Y-%m-%d %H:%M:%S ", tres); + } + pid_t pid = getpid(); + out += snprintf(out, end - out, "%7d ", pid); // ' 38588' va_list args; va_start(args, format); - vfprintf(stderr, format, args); + vsnprintf(out, end - out, format, args); va_end(args); + fputs(buffer, stderr); } - diff --git a/cgit.c b/cgit.c index 26b4045..25e29f9 100644 --- a/cgit.c +++ b/cgit.c @@ -217,6 +217,8 @@ static void config_cb(const char *name, const char *value) ctx.cfg.cache_scanrc_ttl = atoi(value); else if (!strcmp(name, "cache-static-ttl")) ctx.cfg.cache_static_ttl = atoi(value); + else if (!strcmp(name, "cache-lock-timeout")) + ctx.cfg.cache_lock_timeout = atoi(value); else if (!strcmp(name, "client-io-idle-timeout")) ctx.cfg.client_io_idle_timeout = atoi(value); else if (!strcmp(name, "client-io-min-rate")) @@ -388,6 +390,7 @@ static void prepare_context(void) ctx.cfg.cache_scanrc_ttl = 15; ctx.cfg.cache_dynamic_ttl = 5; ctx.cfg.cache_static_ttl = -1; + ctx.cfg.cache_lock_timeout = 10; ctx.cfg.client_io_idle_timeout = 20; ctx.cfg.client_io_min_rate = 500; ctx.cfg.case_sensitive_sort = 1; @@ -887,6 +890,7 @@ static void print_config(FILE *f, const char *prefix) fprintf(f, "%slog-level=%d\n", prefix, ctx.cfg.log_level); fprintf(f, "%sproject-list=%s\n", prefix, ctx.cfg.project_list); fprintf(f, "%sscan-path=%s\n", prefix, ctx.cfg.scan_path); + fprintf(f, "%scache-lock-timeout=%d\n", prefix, ctx.cfg.cache_lock_timeout); fprintf(f, "%sclient-io-idle-timeout=%d\n", prefix, ctx.cfg.client_io_idle_timeout); fprintf(f, "%sclient-io-min-rate=%ld\n", prefix, ctx.cfg.client_io_min_rate); } diff --git a/cgit.h b/cgit.h index f16e501..4aed96c 100644 --- a/cgit.h +++ b/cgit.h @@ -229,6 +229,8 @@ struct cgit_config { int cache_static_ttl; int cache_about_ttl; int cache_snapshot_ttl; + /* cache lock timeout in seconds to acquire the cache lock-file against concurrent processes. Defaults to 10s. */ + int cache_lock_timeout; /* idle timeout in seconds between sending/receiving chunks of the cached body to/from the client. Defaults to 20s. */ int client_io_idle_timeout; /* minimum transfer rate in Bps for sending/receiving a full cached body to/from the client. Defaults to 500 Bps. */ diff --git a/cgitrc.5.txt b/cgitrc.5.txt index a6016ee..d3a8ae8 100644 --- a/cgitrc.5.txt +++ b/cgitrc.5.txt @@ -100,6 +100,10 @@ cache-static-ttl:: version of repository pages accessed with a fixed SHA1. See also: "CACHE". Default value: -1". +cache-lock-timeout:: + Timeout in seconds to acquire the cache lock-file + against concurrent processes. Default value: "10". + client-io-idle-timeout:: IDLE timeout in seconds between sending/receiving chunks of the cached body to/from the client. Default value: "20".
