[PATCH 3/4] Resolve concurrent initial cache fill post lock. Release lock after, fill.
Sven Göthel
sgothel at jausoft.com
Mon Jul 20 00:43:08 UTC 2026
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 <sgothel at jausoft.com>
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 <sgothel at jausoft.com>
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".
More information about the CGit
mailing list