From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18984 invoked by alias); 23 Jan 2015 20:49:25 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 34363 Received: (qmail 22927 invoked from network); 23 Jan 2015 20:49:20 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-0.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE, URIBL_BLACK autolearn=no version=3.3.2 X-Originating-IP: [86.6.153.127] X-Spam: 0 X-Authority: v=2.1 cv=S8BXwecP c=1 sm=1 tr=0 a=39NrsSuza2clQiZR/7fYWQ==:117 a=39NrsSuza2clQiZR/7fYWQ==:17 a=kj9zAlcOel0A:10 a=NLZqzBF-AAAA:8 a=5z0F9b7kAAAA:8 a=wuwgkVytzVxt1ieYCiMA:9 a=9ChGwAoV1pSchsKT:21 a=JFfw-V-8jeJtoqhY:21 a=CjuIK1q_8ugA:10 Date: Fri, 23 Jan 2015 20:49:16 +0000 From: Peter Stephenson To: ondra@mistotebe.net, zsh workers Subject: Re: History file lock contention Message-ID: <20150123204916.0f92c246@ntlworld.com> In-Reply-To: References: <20150123114628.GB10898@mistotebe.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Fri, 23 Jan 2015 12:13:09 +0000 Peter Stephenson wrote: > On 23 January 2015 at 11:46, 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],