cache issue

[John Keeping]

On Sun, Mar 01, 2015 at 06:43:17PM +0000, Bertrand Jacquin wrote:
> On 28/02/2015 12:37, John Keeping wrote:
> > On Sat, Feb 28, 2015 at 12:06:41PM +0000, Bertrand Jacquin wrote:
> >> We are still experiencing the issue. Is there any fixes with newer
> >> releases ?
> > 
> > I have just tried to reproduce this with the latest version and have 
> > not
> > been able to do so, but I'm not aware of any changes that should have 
> > an
> > effect on this (there is one cache change, 6ceba45 Skip cache slot when
> > time-to-live is zero, but that only applies if you set one of the *-ttl
> > values to zero).
> > 
> > The cache timeout logic relies on the mtime of the cache file, so this
> > could be affected by your filesystem, but it sounds like the problem is
> > that the .lock files are not being cleaned up.
> 
> The filesystem here is a ext4 with no specific option except noatime 
> which quiet common.
> 
> > When CGit finds a .lock
> > file for a cache slot it is trying to use it will just serve the stale
> > content, on the assumption that is has only just been replaced.
> 
> So there is so assumption the .lock can be obsolete ?
> 
> > I can't see many ways that you can end up with stale lock files; the
> > only options are:
> > 
> > 1) CGit crashes, in which case there should be some evidence in the
> >    system log.
> 
> That might happend, the cgi can in this case be killed after 60 seconds.

I wonder if we should be doing something like this (which is probably 3
patches if cleaned up, but shows the idea):

-- >8 --
diff --git a/cache.c b/cache.c
index e0bb47a..e7649ad 100644
--- a/cache.c
+++ b/cache.c
@@ -195,6 +195,7 @@ static int unlock_slot(struct cache_slot *slot, int replace_old_slot)
 	else
 		err = unlink(slot->lock_name);
 
+	slot->lock_name = NULL;
 	if (err)
 		return errno;
 
@@ -343,6 +344,14 @@ static int process_slot(struct cache_slot *slot)
 	return err;
 }
 
+static struct cache_slot the_slot;
+
+void cache_cleanup_locks(void)
+{
+	if (the_slot.lock_name)
+		unlock_slot(&the_slot, 0);
+}
+
 /* Print cached content to stdout, generate the content if necessary. */
 int cache_process(int size, const char *path, const char *key, int ttl,
 		  cache_fill_fn fn)
@@ -351,7 +360,6 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	int i;
 	struct strbuf filename = STRBUF_INIT;
 	struct strbuf lockname = STRBUF_INIT;
-	struct cache_slot slot;
 	int result;
 
 	/* If the cache is disabled, just generate the content */
@@ -377,13 +385,13 @@ int cache_process(int size, const char *path, const char *key, int ttl,
 	}
 	strbuf_addbuf(&lockname, &filename);
 	strbuf_addstr(&lockname, ".lock");
-	slot.fn = fn;
-	slot.ttl = ttl;
-	slot.cache_name = filename.buf;
-	slot.lock_name = lockname.buf;
-	slot.key = key;
-	slot.keylen = strlen(key);
-	result = process_slot(&slot);
+	the_slot.fn = fn;
+	the_slot.ttl = ttl;
+	the_slot.cache_name = filename.buf;
+	the_slot.lock_name = lockname.buf;
+	the_slot.key = key;
+	the_slot.keylen = strlen(key);
+	result = process_slot(&the_slot);
 
 	strbuf_release(&filename);
 	strbuf_release(&lockname);
diff --git a/cache.h b/cache.h
index 9392836..f0d1c75 100644
--- a/cache.h
+++ b/cache.h
@@ -28,6 +28,9 @@ extern int cache_process(int size, const char *path, const char *key, int ttl,
 /* List info about all cache entries on stdout */
 extern int cache_ls(const char *path);
 
+/* Cleanup open cache lock files on abnormal exit */
+extern void cache_cleanup_locks(void);
+
 /* Print a message to stdout */
 __attribute__((format (printf,1,2)))
 extern void cache_log(const char *format, ...);
diff --git a/cgit.c b/cgit.c
index d9a8b1f..0912a3d 100644
--- a/cgit.c
+++ b/cgit.c
@@ -1031,6 +1031,26 @@ static int calc_ttl()
 	return ctx.cfg.cache_repo_ttl;
 }
 
+static void cleanup_handler(int signum)
+{
+	cache_cleanup_locks();
+}
+
+static void register_signal_handlers(void)
+{
+	struct sigaction sa = {
+		.sa_handler = cleanup_handler,
+		.sa_flags = SA_RESETHAND,
+	};
+	sigemptyset(&sa.sa_mask);
+
+	sigaction(SIGHUP, &sa, NULL);
+	sigaction(SIGINT, &sa, NULL);
+	sigaction(SIGQUIT, &sa, NULL);
+	sigaction(SIGPIPE, &sa, NULL);
+	sigaction(SIGTERM, &sa, NULL);
+}
+
 int main(int argc, const char **argv)
 {
 	const char *path;
@@ -1039,6 +1059,8 @@ int main(int argc, const char **argv)
 	cgit_init_filters();
 	atexit(cgit_cleanup_filters);
 
+	register_signal_handlers();
+
 	prepare_context();
 	cgit_repolist.length = 0;
 	cgit_repolist.count = 0;