From dc0eb5e49a38da4186f37f89be21983704114567 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 18 Feb 2022 13:17:19 +0000 Subject: [PATCH] =?UTF-8?q?gdbusauthmechanismsha1:=20Don=E2=80=99t=20delet?= =?UTF-8?q?e=20a=20stale=20lock=20file=20if=20it=E2=80=99s=20changed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry loop for acquiring the lock for the authentication cookie file currently tries to acquire the lock for 0.5s, then gives up, assumes the lock file is stale, and deletes it. That’s great if the lock file *is* stale because it’s been left there by a crashed process. It’s not so great if the lock file just happens to have been there every time this process checked, because the cookie file is highly contested while (for example) running lots of parallel unit tests. Check for that situation by comparing the mtime of the lock file and continuing to retry if it’s changed. Signed-off-by: Philip Withnall Fixes: #1929 --- gio/gdbusauthmechanismsha1.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c index 8137b6352..ed5aa3f96 100644 --- a/gio/gdbusauthmechanismsha1.c +++ b/gio/gdbusauthmechanismsha1.c @@ -38,6 +38,7 @@ #include "gdbusauthmechanismsha1.h" #include "gcredentials.h" #include "gdbuserror.h" +#include "glocalfileinfo.h" #include "gioenumtypes.h" #include "gioerror.h" #include "gdbusprivate.h" @@ -509,6 +510,7 @@ _log (const gchar *message, * and was created successfully) */ static gint create_lock_exclusive (const gchar *lock_path, + gint64 *mtime_nsec, GError **error) { int errsv; @@ -518,6 +520,16 @@ create_lock_exclusive (const gchar *lock_path, errsv = errno; if (ret < 0) { + GLocalFileStat stat_buf; + + /* Get the modification time to distinguish between the lock being stale + * or highly contested. */ + if (mtime_nsec != NULL && + g_local_file_stat (lock_path, G_LOCAL_FILE_STAT_FIELD_MTIME, G_LOCAL_FILE_STAT_FIELD_ALL, &stat_buf) == 0) + *mtime_nsec = _g_stat_mtime (&stat_buf) * G_USEC_PER_SEC * 1000 + _g_stat_mtim_nsec (&stat_buf); + else if (mtime_nsec != NULL) + *mtime_nsec = 0; + g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errsv), @@ -538,6 +550,7 @@ keyring_acquire_lock (const gchar *path, gint ret; guint num_tries; int errsv; + gint64 lock_mtime_nsec = 0, lock_mtime_nsec_prev = 0; /* Total possible sleep period = max_tries * timeout_usec = 0.5s */ const guint max_tries = 50; @@ -565,13 +578,21 @@ keyring_acquire_lock (const gchar *path, for (num_tries = 0; num_tries < max_tries; num_tries++) { + lock_mtime_nsec_prev = lock_mtime_nsec; + /* Ignore the error until the final call. */ - ret = create_lock_exclusive (lock, NULL); + ret = create_lock_exclusive (lock, &lock_mtime_nsec, NULL); if (ret >= 0) break; /* sleep 10ms, then try again */ g_usleep (timeout_usec); + + /* If the mtime of the lock file changed, don’t count the retry, as it + * seems like there’s contention between processes for the lock file, + * rather than a stale lock file from a crashed process. */ + if (num_tries > 0 && lock_mtime_nsec != lock_mtime_nsec_prev) + num_tries--; } if (num_tries == max_tries) @@ -594,7 +615,7 @@ keyring_acquire_lock (const gchar *path, _log ("Deleted stale lock file '%s'", lock); /* Try one last time to create it, now that we've deleted the stale one */ - ret = create_lock_exclusive (lock, error); + ret = create_lock_exclusive (lock, NULL, error); if (ret < 0) goto out; }