From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE autolearn=ham autolearn_force=no version=3.4.2 Received: from primenet.com.au (ns1.primenet.com.au [203.24.36.2]) by inbox.vuxu.org (OpenSMTPD) with ESMTP id a8e0fa2b for ; Mon, 6 Jan 2020 17:31:18 +0000 (UTC) Received: (qmail 135 invoked by alias); 6 Jan 2020 17:31:09 -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: List-Unsubscribe: X-Seq: 45249 Received: (qmail 27670 invoked by uid 1010); 6 Jan 2020 17:31:09 -0000 X-Qmail-Scanner-Diagnostics: from nef2.ens.fr by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.102.1/25684. spamassassin: 3.4.2. Clear:RC:0(129.199.96.40):SA:0(-4.2/5.0):. Processed in 2.124183 secs); 06 Jan 2020 17:31:09 -0000 X-Envelope-From: ware@phare.normalesup.org X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: pass (ns1.primenet.com.au: SPF record at ens.fr designates 129.199.96.40 as permitted sender) X-ENS-nef-client: 129.199.129.80 Date: Mon, 6 Jan 2020 18:30:30 +0100 From: Cedric Ware To: dana Cc: "zsh-workers@zsh.org" Subject: Re: [PATCH] Enable sub-second timeout in zsystem flock Message-ID: <20200106173030.eb2pg4rhhgysh35r@phare.normalesup.org> References: <20190729203521.upp5ku3bsr3hsnxq@phare.normalesup.org> <20200104184734.ienw42rwq2xu6aap@phare.normalesup.org> <43775C64-0254-45AB-81AB-B04AE80C4416@dana.is> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <43775C64-0254-45AB-81AB-B04AE80C4416@dana.is> User-Agent: NeoMutt/20170113 (1.7.2) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.4.3 (nef.ens.fr [129.199.96.32]); Mon, 06 Jan 2020 18:30:30 +0100 (CET) dana (Sunday 2020-01-05): > Feedback off the top of my head (haven't tested and don't know whether anyone > else would object to the change): Thanks for the feedback. Revised patch below. > The first thing that occurs to me is that you've now decoupled the polling > interval and time-out, which means that not only can you *manually* specify a > polling interval greater than the time-out, it's very possible for the > *default* interval to be greater than the time-out. In fact any -t value less > than one second is kind of pointless unless you also give -p Not quite pointless: in the last iteration of the retry loop, if the timeout is closer to the current time than the retry interval, zsleep() only waits until the timeout, then there's a last locking attempt, and only then the function fails. So, if the retry interval is longer than the timeout, there will still be 2 locking attempts, one at the start, one at the timeout. One could object that this behavior does not quite follow what one would expect when specifying a retry interval, but as long as the documentation mentions it, this seems reasonable to me. > Also this addition is worded a bit strangely i think. period is an argument to > -p; it isn't used unless -p is. Maybe something like: 'During this period, the > shell will attempt to lock the file every period seconds if the -p option is > given, otherwise once a second.' Something to that effect anyway That's clearer, indeed. Also I changed "-p period" to "-i interval": I had in mind "period-as-in-periodicity", but that was ambiguous. > > + long timeout_retry = 1e6; > > + mnumber timeout_param; > > It's kind of confusing that these are both named 'timeout' when they don't > specifically have anything to do with the time-out As you mentioned in your other email, they're only used when -t is given. Also, I thought I'd reuse the same mnumber variable to parse the arguments of both -t and -i, but I can change that if it breaks the coding style. > > + zlong timeout_retry_tmp = timeout_param.u.d * 1e6; > > zsh mandates C89, you can't declare variables in the middle of a block OK. > > + timeout_retry = (timeout_retry_tmp > LONG_MAX) ? > > + LONG_MAX : timeout_retry_tmp; > > Should *this* kind of thing be an error? I guess there's not really any > error-checking in the existing option-handling code, though What I had in mind was that the retry interval has to be a long because of zsleep(), which on 32-bit systems limits it to about 35 minutes. It seems likely that a user could specify a value that leads to an overflow; but OTOH I thought that it wasn't a problem if locking was attempted more often than specified. Now that I've thought about it again, I added a similar check for the timeout value even though it's a zlong, and I documented that the interval is silently capped at LONG_MAX microseconds. Do you think it should be reported as an error instead? Also, I assumed that zlong was at least 64-bit; can it happen that it be only 32-bit on some systems? That would be a problem to implement the microsecond clock function. > > +time_clock_us(void) > > Feels like a weird function name, but i don't have a better suggestion Likewise. > btw, it would probably be ideal if shtimer (the variable the shell uses for > the time built-in, the SECONDS parameter, &c.) was monotonic as well. But > that's a separate thing Maybe. I believe CLOCK_MONOTONIC stops when the system is suspended; Linux has CLOCK_BOOTIME that adds the suspend time, but it's not portable. I don't know what would surprise users more between having SECONDS run backwards (which it could now), and not including suspend time (if it was changed to CLOCK_MONOTONIC). > The test stuff is in the Test directory in the repo. There don't seem to be > any existing tests for zsystem, so you would have to make like a > V13zsystem.ztst (see the other V* files for inspiration). Not sure what the > tests would actually do, though. Maybe you could call it with low -p/-t values > and use SECONDS to ensure that it times out roughly when it's supposed to? Thanks for the pointer. Yes, that could be done, though I don't have the time right now. What I actually had in mind was a test suite I could run to check that I didn't break anything elsewhere. Which is "make test", I should have thought of it. The current tests are still successful with the patch. Best regards, Cedric Ware. diff -ru zsh-5.7.1-test-2.orig/Doc/Zsh/mod_system.yo zsh-5.7.1-test-2/Doc/Zsh/mod_system.yo --- zsh-5.7.1-test-2.orig/Doc/Zsh/mod_system.yo 2019-08-17 23:14:27.000000000 +0200 +++ zsh-5.7.1-test-2/Doc/Zsh/mod_system.yo 2020-01-06 17:57:35.835049585 +0100 @@ -166,7 +166,7 @@ printed in the last case, but the parameter tt(ERRNO) will reflect the error that occurred. ) -xitem(tt(zsystem flock) [ tt(-t) var(timeout) ] [ tt(-f) var(var) ] [tt(-er)] var(file)) +xitem(tt(zsystem flock) [ tt(-t) var(timeout) ] [ tt(-i) var(interval) ] [ tt(-f) var(var) ] [tt(-er)] var(file)) item(tt(zsystem flock -u) var(fd_expr))( The builtin tt(zsystem)'s subcommand tt(flock) performs advisory file locking (via the manref(fcntl)(2) system call) over the entire contents @@ -196,9 +196,16 @@ By default the shell waits indefinitely for the lock to succeed. The option tt(-t) var(timeout) specifies a timeout for the lock in -seconds; currently this must be an integer. The shell will attempt -to lock the file once a second during this period. If the attempt -times out, status 2 is returned. +seconds; fractional seconds are allowed. During this period, the +shell will attempt to lock the file every var(interval) seconds +if the tt(-i) var(interval) option is given, otherwise once a second. +(This var(interval) is shortened before the last attempt if needed, +so that the shell waits only until the var(timeout) and not longer.) +If the attempt times out, status 2 is returned. + +(Note: var(interval) is silently capped at LONG_MAX microseconds. +This is many millenia on 64-bit systems, but only about 35 minutes +on 32-bit systems.) If the option tt(-e) is given, the file descriptor for the lock is preserved when the shell uses tt(exec) to start a new process; diff -ru zsh-5.7.1-test-2.orig/Src/Modules/system.c zsh-5.7.1-test-2/Src/Modules/system.c --- zsh-5.7.1-test-2.orig/Src/Modules/system.c 2019-08-17 23:14:27.000000000 +0200 +++ zsh-5.7.1-test-2/Src/Modules/system.c 2020-01-06 18:14:41.045767126 +0100 @@ -532,6 +532,9 @@ { int cloexec = 1, unlock = 0, readlock = 0; zlong timeout = -1; + double timeout_tmp; + long timeout_retry = 1e6; + mnumber timeout_param; char *fdvar = NULL; #ifdef HAVE_FCNTL_H struct flock lck; @@ -583,7 +586,38 @@ } else { optarg = *args++; } - timeout = mathevali(optarg); + timeout_param = matheval(optarg); + if (!(timeout_param.type & MN_FLOAT)) { + timeout_param.type = MN_FLOAT; + timeout_param.u.d = (double)timeout_param.u.l; + } + timeout_tmp = timeout_param.u.d * 1e6; + timeout = (timeout_tmp > ZLONG_MAX / 2) ? + ZLONG_MAX / 2 : (zlong)timeout_tmp; + break; + + case 'i': + /* retry interval in seconds */ + if (optptr[1]) { + optarg = optptr + 1; + optptr += strlen(optarg) - 1; + } else if (!*args) { + zwarnnam(nam, + "flock: option %c requires " + "a numeric retry interval", + opt); + return 1; + } else { + optarg = *args++; + } + timeout_param = matheval(optarg); + if (!(timeout_param.type & MN_FLOAT)) { + timeout_param.type = MN_FLOAT; + timeout_param.u.d = (double)timeout_param.u.l; + } + timeout_tmp = timeout_param.u.d * 1e6; + timeout_retry = (timeout_tmp > LONG_MAX) ? + LONG_MAX : (long)timeout_tmp; break; case 'u': @@ -647,7 +681,8 @@ lck.l_len = 0; /* lock the whole file */ if (timeout > 0) { - time_t end = time(NULL) + (time_t)timeout; + zlong now; + zlong end = time_clock_us() + timeout; while (fcntl(flock_fd, F_SETLK, &lck) < 0) { if (errflag) { zclose(flock_fd); @@ -658,11 +693,15 @@ zwarnnam(nam, "failed to lock file %s: %e", args[0], errno); return 1; } - if (time(NULL) >= end) { + now = time_clock_us(); + if (now >= end) { zclose(flock_fd); return 2; } - sleep(1); + if (now + timeout_retry > end) { + timeout_retry = end - now; + } + zsleep(timeout_retry); } } else { while (fcntl(flock_fd, timeout == 0 ? F_SETLK : F_SETLKW, &lck) < 0) { diff -ru zsh-5.7.1-test-2.orig/Src/utils.c zsh-5.7.1-test-2/Src/utils.c --- zsh-5.7.1-test-2.orig/Src/utils.c 2019-12-21 09:29:14.000000000 +0100 +++ zsh-5.7.1-test-2/Src/utils.c 2020-01-03 21:13:34.023588812 +0100 @@ -2749,6 +2749,26 @@ } /* + * Return the current time in microseconds, using the system's + * monotonic clock if supported, the wall clock if not. + */ + +/**/ +zlong +time_clock_us(void) +{ +#if defined(HAS_CLOCK_GETTIME) && defined(CLOCK_MONOTONIC) + struct timespec ts; + clock_gettime(CLOCK_MONOTONIC, &ts); + return ts.tv_sec * (zlong)1e6 + ts.tv_nsec / 1000; +#else + struct timeval tv; + gettimeofday(&tv, NULL); + return tv.tv_sec * (zlong)1e6 + tv.tv_usec; +#endif +} + +/* * 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.