Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

Re: History file lock contention



On Fri, 23 Jan 2015 12:13:09 +0000
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx> wrote:
> On 23 January 2015 at 11:46, <ondra@xxxxxxxxxxxxx> wrote:
> > when running zsh from a terminal multiplexer like tmux, it is possible
> > (and not uncommon) to close several processes at the exact same moment.
> > In that case, all of them try to sync their history and all but one fail
> > to acquire the lock. So far so good.
> >
> > However, all of these call nanosleep({1,0}), and since we might be on a
> > multicore system, they might just be scheduled at the same time again,
> > only one of them will progress and the scenario repeats. What the user
> > sees is the terminals not disappearing immediately, but one after the
> > other, a second per process.
>
> A better scheme would be to start with a small wait time, says 0.1 seconds,
> with a random back off, with a maximum length of say the same time again,
> and then double the base time each time we back off, but keeping the
> same maximum time of 10 seconds since we first tried before reporting
> a failure.  We can then tweak this as necessary.  Or maybe just
> having a random backoff with doubling maximum and no fixed
> base time is good enough for typical problems.

The following doesn't appear to be too fundamentally broken, however I
don't have a set-up that's particularly likely to trigger problems.  It
would be good if other people who think they do could try it out before
I commit it --- although I don't think it's too far off from being
usable by bleeding edge types.

pws

P.S. Looks like I forgot to "remove formatting" from webmail last time.
Back with proper mail.

diff --git a/Src/hist.c b/Src/hist.c
index 11d9722..05dcdb6 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -2610,7 +2610,8 @@ static int
 flockhistfile(char *fn, int keep_trying)
 {
     struct flock lck;
-    int ctr = keep_trying ? 9 : 0;
+    long sleep_us = 0x4000000; /* about 67 ms */
+    time_t end_time;
 
     if (flock_fd >= 0)
 	return 0; /* already locked */
@@ -2623,13 +2624,22 @@ flockhistfile(char *fn, int keep_trying)
     lck.l_start = 0;
     lck.l_len = 0;  /* lock the whole file */
 
+    /*
+     * Timeout is ten seconds.
+     */
+    end_time = time(NULL) + (time_t)10;
     while (fcntl(flock_fd, F_SETLKW, &lck) == -1) {
-	if (--ctr < 0) {
+	if (!keep_trying || time(NULL) >= end_time ||
+	    /*
+	     * Randomise wait to minimise clashes with shells exiting at
+	     * the same time.
+	     */
+	    !zsleep_random(sleep_us, end_time)) {
 	    close(flock_fd);
 	    flock_fd = -1;
 	    return 1;
 	}
-	sleep(1);
+	sleep_us <<= 1;
     }
 
     return 0;
@@ -2865,7 +2875,7 @@ savehistfile(char *fn, int err, int writeflags)
 static int lockhistct;
 
 static int
-checklocktime(char *lockfile, time_t then)
+checklocktime(char *lockfile, long *sleep_usp, time_t then)
 {
     time_t now = time(NULL);
 
@@ -2875,9 +2885,19 @@ checklocktime(char *lockfile, time_t then)
 	return -1;
     }
 
-    if (now - then < 10)
-	sleep(1);
-    else
+    if (now - then < 10) {
+	/*
+	 * To give the effect of a gradually increasing backoff,
+	 * we'll sleep a period based on the time we've spent so far.
+	 */
+	DPUTS(now < then, "time flowing backwards through history");
+	/*
+	 * Randomise to minimise clashes with shells exiting at the same
+	 * time.
+	 */
+	(void)zsleep_random(*sleep_usp, then + 10);
+	*sleep_usp <<= 1;
+    } else
 	unlink(lockfile);
 
     return 0;
@@ -2894,6 +2914,7 @@ lockhistfile(char *fn, int keep_trying)
 {
     int ct = lockhistct;
     int ret = 0;
+    long sleep_us = 0x4000000; /* about 67 ms */
 
     if (!fn && !(fn = getsparam("HISTFILE")))
 	return 1;
@@ -2937,7 +2958,7 @@ lockhistfile(char *fn, int keep_trying)
 		    continue;
 		break;
 	    }
-	    if (checklocktime(lockfile, sb.st_mtime) < 0) {
+	    if (checklocktime(lockfile, &sleep_us, sb.st_mtime) < 0) {
 		ret = 1;
 		break;
 	    }
@@ -2965,7 +2986,7 @@ lockhistfile(char *fn, int keep_trying)
 			continue;
 		    ret = 2;
 		} else {
-		    if (checklocktime(lockfile, sb.st_mtime) < 0) {
+		    if (checklocktime(lockfile, &sleep_us, sb.st_mtime) < 0) {
 			ret = 1;
 			break;
 		    }
@@ -2993,7 +3014,7 @@ lockhistfile(char *fn, int keep_trying)
 		ret = 2;
 		break;
 	    }
-	    if (checklocktime(lockfile, sb.st_mtime) < 0) {
+	    if (checklocktime(lockfile, &sleep_us, sb.st_mtime) < 0) {
 		ret = 1;
 		break;
 	    }
diff --git a/Src/utils.c b/Src/utils.c
index cf18f12..6ee2ec6 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2261,6 +2261,10 @@ setblock_stdin(void)
  * Note that apart from setting (and restoring) non-blocking input,
  * this function does not change the input mode.  The calling function
  * should have set cbreak mode if necessary.
+ *
+ * fd may be -1 to sleep until the timeout in microseconds.  This is a
+ * fallback for old systems that don't have nanosleep().  Some very old
+ * systems might not have select: get with it, daddy-o.
  */
 
 /**/
@@ -2282,6 +2286,8 @@ read_poll(int fd, int *readchar, int polltty, zlong microseconds)
     struct ttyinfo ti;
 #endif
 
+     if (fd < 0)
+	 polltty = 0;		/* no tty to poll */
 
 #if defined(HAS_TIO) && !defined(__CYGWIN__)
     /*
@@ -2303,7 +2309,7 @@ read_poll(int fd, int *readchar, int polltty, zlong microseconds)
      * as plausible as it sounds, but it seems the right way to guess.
      *		pws 2000/06/26
      */
-    if (polltty) {
+    if (fd >= 0) {
 	gettyinfo(&ti);
 	if ((polltty = ti.tio.c_cc[VMIN])) {
 	    ti.tio.c_cc[VMIN] = 0;
@@ -2319,16 +2325,24 @@ read_poll(int fd, int *readchar, int polltty, zlong microseconds)
     expire_tv.tv_sec = (int) (microseconds / (zlong)1000000);
     expire_tv.tv_usec = microseconds % (zlong)1000000;
     FD_ZERO(&foofd);
-    FD_SET(fd, &foofd);
-    ret = select(fd+1, (SELECT_ARG_2_T) &foofd, NULL, NULL, &expire_tv);
+    if (fd > -1) {
+	FD_SET(fd, &foofd);
+	ret = select(fd+1, (SELECT_ARG_2_T) &foofd, NULL, NULL, &expire_tv);
+    } else
+	ret = select(0, NULL, NULL, NULL, &expire_tv);
 #else
+    if (fd < 0) {
+	/* OK, can't do that.  Just quietly sleep for a second. */
+	sleep(1);
+	return 1;
+    }
 #ifdef FIONREAD
     if (ioctl(fd, FIONREAD, (char *) &val) == 0)
 	ret = (val > 0);
 #endif
 #endif
 
-    if (ret < 0) {
+    if (fd >= 0 && ret < 0) {
 	/*
 	 * Final attempt: set non-blocking read and try to read a character.
 	 * Praise Bill, this works under Cygwin (nothing else seems to).
@@ -2350,6 +2364,80 @@ read_poll(int fd, int *readchar, int polltty, zlong microseconds)
     return (ret > 0);
 }
 
+/*
+ * Sleep for the given number of microseconds --- must be within
+ * range of a long at the moment, but this is only used for
+ * limited internal purposes.
+ */
+
+/**/
+int
+zsleep(long us)
+{
+#ifdef HAVE_NANOSLEEP
+    struct timespec sleeptime;
+
+    sleeptime.tv_sec = (time_t)us / (time_t)1000000;
+    sleeptime.tv_nsec = (us % 1000000L) * 1000L;
+    for (;;) {
+	struct timespec rem;
+	int ret = nanosleep(&sleeptime, &rem);
+
+	if (ret == 0)
+	    return 1;
+	else if (errno != EINTR)
+	    return 0;
+	sleeptime = rem;
+    }
+#else
+    int dummy;
+    return read_poll(-1, &dummy, 0, us);
+#endif
+}
+
+/**
+ * Sleep for time (fairly) randomly up to max_us microseconds.
+ * Don't let the wallclock time extend beyond end_time.
+ * Return 1 if that seemed to work, else 0.
+ *
+ * For best results max_us should be a multiple of 2**16 or large
+ * enough that it doesn't matter.
+ */
+
+/**/
+int
+zsleep_random(long max_us, time_t end_time)
+{
+    int r;
+    time_t now = time(NULL);
+
+    /*
+     * Randomish backoff.  Doesn't need to be fundamentally
+     * unpredictable, just probably unlike the value another
+     * exiting shell is using.  On some systems the bottom 16
+     * bits aren't that random but the use here doesn't
+     * really care.
+     */
+    r = (long)(rand() & 0xFFFF);
+    /*
+     * Turn this into a fraction of sleep_us.  Again, this
+     * doesn't need to be particularly accurate and the base time
+     * is sufficient that we can do the division first and not
+     * worry about the range.
+     */
+    r = (max_us >> 16) * r;
+    /*
+     * Don't sleep beyond timeout.
+     * Not that important as timeout is ridiculously long, but
+     * if there's an interface, interface to it...
+     */
+    while (now + r > end_time)
+	r >>= 1;
+    if (r) /* pedantry */
+	return zsleep(r);
+    return 0;
+}
+
 /**/
 int
 checkrmall(char *s)
diff --git a/configure.ac b/configure.ac
index 8ea9737..bfc02b2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1299,7 +1299,8 @@ AC_CHECK_FUNCS(strftime strptime mktime timelocal \
 	       gdbm_open getxattr \
 	       realpath canonicalize_file_name \
 	       symlink getcwd \
-	       cygwin_conv_path)
+	       cygwin_conv_path \
+	       nanosleep)
 AC_FUNC_STRCOLL
 
 AH_TEMPLATE([REALPATH_ACCEPTS_NULL],



Messages sorted by: Reverse Date, Date, Thread, Author