From: Cedric Ware <cedric.ware__bml@normalesup.org>
To: dana <dana@dana.is>
Cc: "zsh-workers@zsh.org" <zsh-workers@zsh.org>
Subject: Re: [PATCH] Enable sub-second timeout in zsystem flock
Date: Mon, 6 Jan 2020 18:30:30 +0100 [thread overview]
Message-ID: <20200106173030.eb2pg4rhhgysh35r@phare.normalesup.org> (raw)
In-Reply-To: <43775C64-0254-45AB-81AB-B04AE80C4416@dana.is>
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.
next prev parent reply other threads:[~2020-01-06 17:31 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-29 20:35 Cedric Ware
2019-07-29 22:25 ` Bart Schaefer
2020-01-04 18:47 ` Cedric Ware
2020-01-05 18:42 ` dana
2020-01-05 21:49 ` dana
2020-01-06 17:30 ` Cedric Ware [this message]
2020-01-06 17:36 ` Peter Stephenson
2020-01-07 3:48 ` dana
2020-01-11 15:41 ` Cedric Ware
2020-01-11 19:36 ` dana
2020-01-12 4:25 ` dana
2020-03-08 18:39 ` Cedric Ware
2020-03-12 18:46 ` dana
2020-03-12 19:13 ` dana
2020-03-14 21:04 ` Cedric Ware
2020-03-15 0:50 ` Daniel Shahaf
2020-03-15 1:04 ` dana
2020-03-15 16:03 ` Cedric Ware
2020-03-15 16:54 ` Daniel Shahaf
2020-03-15 17:35 ` Peter Stephenson
2020-03-15 18:36 ` Cedric Ware
2020-03-15 19:13 ` Daniel Shahaf
2020-04-13 21:34 ` Cedric Ware
2020-04-14 11:47 ` Daniel Shahaf
2020-04-14 20:21 ` Cedric Ware
2020-04-15 1:15 ` Daniel Shahaf
2020-04-15 2:05 ` dana
2020-04-16 4:24 ` Daniel Shahaf
2020-04-18 16:32 ` Cedric Ware
2020-04-20 17:28 ` dana
2020-04-20 22:17 ` Cedric Ware
2020-03-15 1:04 ` Daniel Shahaf
2020-03-13 14:26 ` dana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200106173030.eb2pg4rhhgysh35r@phare.normalesup.org \
--to=cedric.ware__bml@normalesup.org \
--cc=dana@dana.is \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).