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;
_______________________________________________
CGit mailing list
CGit@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/cgit

Reply via email to