zsh-workers
 help / color / Atom feed
* [PATCH] Enable sub-second timeout in zsystem flock
@ 2019-07-29 20:35 Cedric Ware
  2019-07-29 22:25 ` Bart Schaefer
  0 siblings, 1 reply; 11+ messages in thread
From: Cedric Ware @ 2019-07-29 20:35 UTC (permalink / raw)
  To: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]


	Hello,

I'm interested in using the zsystem flock function in the zsh/system
module to lock files, with a timeout if the lock is already taken, but
currently the timeout value has a whole-second granularity and I'd like
to specify sub-second timeouts.

The attached patch (against zsh 5.7.1) modifies the bin_zsystem_flock()
function, allowing the -t option to take floating-point parameters, and
handles the timeout with microsecond granularity.  It also adds option
-p to control the retrying period, which was previously hardcoded to
1 second.

I kept the same function's logic of periodically retrying to acquire
the lock in non-blocking mode.  However, there may be a better way to
implement this timeout: set a timer such as setitimer(2) which sends
a SIGALRM on timeout, trap SIGALRM without SA_RESTART, then acquire the
lock in blocking mode; fcntl() will then block until SIGALRM is caught,
after which errno will be EINTR.  Unfortunately, lacking knowledge of
zsh internals especially signal handling, I'm not confident that I can
reimplement it this way by myself.

The patch modifies Src/Modules/system.c and also Src/utils.c (to add
a function akin to time(NULL) but that returns microseconds, using
clock_gettime(CLOCK_MONOTONIC) if supported, or else gettimeofday()).

I did not modify the documentation yet, not being sure of people being
interested in this feature, nor of whether I should edit just the
Doc/Zsh/mod_system.yo file, or also Doc/{zshmodules.1,zsh.texi} or
others.

Also, I'm afraid that I only did minimal testing.  I checked that
invoking zsystem flock with various values for -t and -p seems to do
what I intended.  Is there a more rigorous test framework for this
kind of thing?

					Thanks, best regards,
					Cedric Ware.

[-- Attachment #2: zsh-5.7.1-flock-timeout.patch --]
[-- Type: text/x-diff, Size: 3047 bytes --]

diff -ur zsh-5.7.1.orig/Src/Modules/system.c zsh-5.7.1/Src/Modules/system.c
--- zsh-5.7.1.orig/Src/Modules/system.c	2018-12-14 08:50:17.000000000 +0100
+++ zsh-5.7.1/Src/Modules/system.c	2019-07-29 20:19:48.835060428 +0200
@@ -532,6 +532,8 @@
 {
     int cloexec = 1, unlock = 0, readlock = 0;
     zlong timeout = -1;
+    long timeout_retry = 1e6;
+    mnumber timeout_param;
     char *fdvar = NULL;
 #ifdef HAVE_FCNTL_H
     struct flock lck;
@@ -583,7 +585,35 @@
 		} 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 = (zlong)(timeout_param.u.d * 1e6);
+		break;
+
+	    case 'p':
+		/* retry period in seconds */
+		if (optptr[1]) {
+		    optarg = optptr + 1;
+		    optptr += strlen(optarg) - 1;
+		} else if (!*args) {
+		    zwarnnam(nam,
+			     "flock: option %c requires a numeric retry period",
+			     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;
+		}
+		zlong timeout_retry_tmp = timeout_param.u.d * 1e6;
+		timeout_retry = (timeout_retry_tmp > LONG_MAX) ?
+		    LONG_MAX : timeout_retry_tmp;
 		break;
 
 	    case 'u':
@@ -647,7 +677,7 @@
     lck.l_len = 0;  /* lock the whole file */
 
     if (timeout > 0) {
-	time_t end = time(NULL) + (time_t)timeout;
+	zlong end = time_clock_us() + timeout;
 	while (fcntl(flock_fd, F_SETLK, &lck) < 0) {
 	    if (errflag) {
                 zclose(flock_fd);
@@ -658,11 +688,15 @@
 		zwarnnam(nam, "failed to lock file %s: %e", args[0], errno);
 		return 1;
 	    }
-	    if (time(NULL) >= end) {
+	    zlong 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 -ur zsh-5.7.1.orig/Src/utils.c zsh-5.7.1/Src/utils.c
--- zsh-5.7.1.orig/Src/utils.c	2019-02-01 01:37:34.000000000 +0100
+++ zsh-5.7.1/Src/utils.c	2019-07-29 20:24:57.827073902 +0200
@@ -2724,6 +2724,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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2019-07-29 20:35 [PATCH] Enable sub-second timeout in zsystem flock Cedric Ware
@ 2019-07-29 22:25 ` Bart Schaefer
  2020-01-04 18:47   ` Cedric Ware
  0 siblings, 1 reply; 11+ messages in thread
From: Bart Schaefer @ 2019-07-29 22:25 UTC (permalink / raw)
  To: Cedric Ware; +Cc: zsh-workers

On Mon, Jul 29, 2019 at 1:36 PM Cedric Ware
<cedric.ware__bml@normalesup.org> wrote:
>
> The attached patch (against zsh 5.7.1) modifies the bin_zsystem_flock()

In future, please either inline patches or attach them as text/plain
(e.g. with a ".txt" extension instead of ".patch").  Thanks!

> I kept the same function's logic of periodically retrying to acquire
> the lock in non-blocking mode.  However, there may be a better way to
> implement this timeout: set a timer such as setitimer(2) which sends
> a SIGALRM on timeout

Unfortunately that technique can't be used because ALRM is a signal
that the user is allowed to trap via the "trap" command or TRAPALRM
function, and which is controlled by the TMOUT variable which is also
user settable.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2019-07-29 22:25 ` Bart Schaefer
@ 2020-01-04 18:47   ` Cedric Ware
  2020-01-05 18:42     ` dana
  0 siblings, 1 reply; 11+ messages in thread
From: Cedric Ware @ 2020-01-04 18:47 UTC (permalink / raw)
  To: zsh-workers


	Hello,

This is a re-send/update of a patch I'd proposed some time ago
(see http://www.zsh.org/mla/workers/2019/msg00620.html), to allow a
sub-second granularity for the zsystem flock function's timeout in the
zsh/system module.  This version of the patch is against 5.7.1-test-2,
and also updates the documentation in Doc/Zsh/mod_system.yo .

At the time, I'd been answered:

Bart Schaefer (Monday 2019-07-29):
> In future, please either inline patches or attach them as text/plain
> (e.g. with a ".txt" extension instead of ".patch").  Thanks!

Sure, please find it at the end of this mail.

> > implement this timeout: set a timer such as setitimer(2) which sends
> > a SIGALRM on timeout
> 
> Unfortunately that technique can't be used because ALRM is a signal
> that the user is allowed to trap via the "trap" command or TRAPALRM
> function, and which is controlled by the TMOUT variable which is also
> user settable.

Indeed, I see now.  But, to be clear, I didn't use that technique here.
That was just speculation.  With this patch, the function still polls
the lock periodically as before, except now the period can be specified
instead of being hardcoded to 1 second.

Also, I did some testing, but only checked that invoking zsystem flock
with various values for -t and -p seemed to do what I intended.
Is there a more rigorous test framework for this kind of thing, or
anything else to be done that might help this patch be included?

					Thanks, 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-03 21:12:34.623024717 +0100
@@ -196,9 +196,10 @@
 
 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.  The shell will attempt
+to lock the file every var(period) seconds during this period:
+once a second by default, or according to the  value set by option
+tt(-p).  If the attempt times out, status 2 is returned.
 
 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-03 21:13:34.019588503 +0100
@@ -532,6 +532,8 @@
 {
     int cloexec = 1, unlock = 0, readlock = 0;
     zlong timeout = -1;
+    long timeout_retry = 1e6;
+    mnumber timeout_param;
     char *fdvar = NULL;
 #ifdef HAVE_FCNTL_H
     struct flock lck;
@@ -583,7 +585,35 @@
 		} 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 = (zlong)(timeout_param.u.d * 1e6);
+		break;
+
+	    case 'p':
+		/* retry period in seconds */
+		if (optptr[1]) {
+		    optarg = optptr + 1;
+		    optptr += strlen(optarg) - 1;
+		} else if (!*args) {
+		    zwarnnam(nam,
+			     "flock: option %c requires a numeric retry period",
+			     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;
+		}
+		zlong timeout_retry_tmp = timeout_param.u.d * 1e6;
+		timeout_retry = (timeout_retry_tmp > LONG_MAX) ?
+		    LONG_MAX : timeout_retry_tmp;
 		break;
 
 	    case 'u':
@@ -647,7 +677,7 @@
     lck.l_len = 0;  /* lock the whole file */
 
     if (timeout > 0) {
-	time_t end = time(NULL) + (time_t)timeout;
+	zlong end = time_clock_us() + timeout;
 	while (fcntl(flock_fd, F_SETLK, &lck) < 0) {
 	    if (errflag) {
                 zclose(flock_fd);
@@ -658,11 +688,15 @@
 		zwarnnam(nam, "failed to lock file %s: %e", args[0], errno);
 		return 1;
 	    }
-	    if (time(NULL) >= end) {
+	    zlong 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  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
  0 siblings, 2 replies; 11+ messages in thread
From: dana @ 2020-01-05 18:42 UTC (permalink / raw)
  To: Cedric Ware; +Cc: zsh-workers

On 4 Jan 2020, at 12:47, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> This is a re-send/update of a patch I'd proposed some time ago

Feedback off the top of my head (haven't tested and don't know whether anyone
else would object to the change):

> +seconds; fractional seconds are allowed.  The shell will attempt
> +to lock the file every var(period) seconds during this period:
> +once a second by default, or according to the  value set by option
> +tt(-p).

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

I'm not sure how you'd address this functionality-wise. Should it be an error?
idk. It should at least be mentioned in the documentation, though

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

> +    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

> +		zlong timeout_retry_tmp = timeout_param.u.d * 1e6;

zsh mandates C89, you can't declare variables in the middle of a block

> +		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

> +time_clock_us(void)

Feels like a weird function name, but i don't have a better suggestion

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

> Is there a more rigorous test framework for this kind of thing, or
> anything else to be done that might help this patch be included?

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?

dana


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2020-01-05 18:42     ` dana
@ 2020-01-05 21:49       ` dana
  2020-01-06 17:30       ` Cedric Ware
  1 sibling, 0 replies; 11+ messages in thread
From: dana @ 2020-01-05 21:49 UTC (permalink / raw)
  To: Cedric Ware; +Cc: zsh-workers

On 5 Jan 2020, at 12:42, dana <dana@dana.is> wrote:
> It's kind of confusing that these are both named 'timeout' when they don't
> specifically have anything to do with the time-out

Actually, this caught my eye again and i realised that they're named that way
because the polling interval is only used when you give a time-out. So never
mind this part, the confusion was mine

dana


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2020-01-05 18:42     ` dana
  2020-01-05 21:49       ` dana
@ 2020-01-06 17:30       ` Cedric Ware
  2020-01-06 17:36         ` Peter Stephenson
  2020-01-07  3:48         ` dana
  1 sibling, 2 replies; 11+ messages in thread
From: Cedric Ware @ 2020-01-06 17:30 UTC (permalink / raw)
  To: dana; +Cc: zsh-workers


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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2020-01-06 17:30       ` Cedric Ware
@ 2020-01-06 17:36         ` Peter Stephenson
  2020-01-07  3:48         ` dana
  1 sibling, 0 replies; 11+ messages in thread
From: Peter Stephenson @ 2020-01-06 17:36 UTC (permalink / raw)
  To: zsh-workers, Cedric Ware

On Mon, 2020-01-06 at 18:30 +0100, Cedric Ware wrote:
> 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.

That shouldn't happen unless the configuration script can't actually
find a 64-bit type, implying either the compiler doesn't support it or
it's not using standard types.  I think you can reasonably consider that
such a failure on a system that actually does support 64-bit integers
isn't your problem.

pws


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2020-01-06 17:30       ` Cedric Ware
  2020-01-06 17:36         ` Peter Stephenson
@ 2020-01-07  3:48         ` dana
  2020-01-11 15:41           ` Cedric Ware
  1 sibling, 1 reply; 11+ messages in thread
From: dana @ 2020-01-07  3:48 UTC (permalink / raw)
  To: Cedric Ware; +Cc: zsh-workers

On 6 Jan 2020, at 11:30, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> 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.

Oh, i missed that, sorry. That doesn't seem so bad.

On 6 Jan 2020, at 11:30, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> 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.

It's fine afaik

On 6 Jan 2020, at 11:30, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> 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?

Like i said, it doesn't seem to do much error-checking anywhere else, so i
guess it's consistent... but if it were me i'd probably make any out-of-range
value return an error, yeah.

On 6 Jan 2020, at 11:30, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> 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).

Since it's mainly used for timing things i think that'd be fine, but maybe
there's something i'm not considering.

On 6 Jan 2020, at 11:30, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> 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.

I'm slightly busy again too but i could try to write a script for it later.
It'd be going into 5.9 anyway

dana


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2020-01-07  3:48         ` dana
@ 2020-01-11 15:41           ` Cedric Ware
  2020-01-11 19:36             ` dana
  0 siblings, 1 reply; 11+ messages in thread
From: Cedric Ware @ 2020-01-11 15:41 UTC (permalink / raw)
  To: dana; +Cc: zsh-workers


dana (Monday 2020-01-06):
> Like i said, it doesn't seem to do much error-checking anywhere else, so i
> guess it's consistent... but if it were me i'd probably make any out-of-range
> value return an error, yeah.

Easy enough, except that producing a useful error message, with the
actual maximum value, is trickier than I'd expected.

zerrmsg() (and thus zwarnnam()) only supports displaying a long (%L)
if DEBUG is enabled (see "#ifdef DEBUG" lines 290 and 328 of
Src/utils.c), and no floating-point types.  Is this deliberate?
I can see why floats would be harder to support, but long seems
harmless.

If %L can be enabled in zerrmsg() then, getting back to zsystem/flock,
if there's an overflow on the value of the retry interval, it's easy to
display an error message saying that the value is limited to
LONG_MAX / 1000000L seconds (slightly underestimated for display
purposes).  As for an overflow on the (64-bit) timeout value, it's
harder to display the maximum value, but then users are less likely to
stumble into an overflow so I'd leave it silent and perhaps document it.

If not, there's always the possibility of displaying a generic "value
too large" message.


dana (Monday 2020-01-06):
> On 6 Jan 2020, at 11:30, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> > 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.
> 
> I'm slightly busy again too but i could try to write a script for it later.

As I've never done this before, I'd feel more confident adding tests to
an existing script than trying to learn and write one from scratch.

					Thanks, best regards,
					Cedric Ware.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2020-01-11 15:41           ` Cedric Ware
@ 2020-01-11 19:36             ` dana
  2020-01-12  4:25               ` dana
  0 siblings, 1 reply; 11+ messages in thread
From: dana @ 2020-01-11 19:36 UTC (permalink / raw)
  To: Cedric Ware; +Cc: zsh-workers

On 11 Jan 2020, at 09:41, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> zerrmsg() (and thus zwarnnam()) only supports displaying a long (%L)
> if DEBUG is enabled (see "#ifdef DEBUG" lines 290 and 328 of
> Src/utils.c), and no floating-point types.  Is this deliberate?

It was deliberate at one time. Daniel actually changed it in master a week or
two ago, so it should be usable now. I don't even think it needs to be that
elaborate though, 'invalid value' or whatever would be fine

On 11 Jan 2020, at 09:41, Cedric Ware <cedric.ware__bml@normalesup.org> wrote:
> As I've never done this before, I'd feel more confident adding tests to
> an existing script than trying to learn and write one from scratch.

np. I should have time this week end actually, will get back to you

dana


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Enable sub-second timeout in zsystem flock
  2020-01-11 19:36             ` dana
@ 2020-01-12  4:25               ` dana
  0 siblings, 0 replies; 11+ messages in thread
From: dana @ 2020-01-12  4:25 UTC (permalink / raw)
  To: Cedric Ware; +Cc: zsh-workers

On 11 Jan 2020, at 13:36, dana <dana@dana.is> wrote:
> I should have time this week end actually, will get back to you

Here's a basic, almost-does-nothing test script for it. Obv need to change the
expected error messages in the second test to whatever you decide on. You can
run it with `make check TESTNUM=V13`

dana


diff --git a/Test/V13system.ztst b/Test/V13system.ztst
new file mode 100644
index 000000000..e3faca37d
--- /dev/null
+++ b/Test/V13system.ztst
@@ -0,0 +1,30 @@
+# Test zsh/system module
+
+%prep
+
+  if zmodload -s zsh/system; then
+    tst_dir=V13.tmp
+    mkdir -p -- $tst_dir
+  else
+    ZTST_unimplemented='the zsh/system module is not available'
+  fi
+
+%test
+
+  (
+    : > $tst_dir/file
+    zsystem flock -t 0.1 -i 0.1 $tst_dir/file
+  )
+0:zsystem flock valid time arguments
+
+  (
+    zsystem flock -t -1                  $tst_dir/file ||
+    zsystem flock -t 9999999999999999999 $tst_dir/file ||
+    zsystem flock -i -1                  $tst_dir/file ||
+    zsystem flock -i 9999999999999999999 $tst_dir/file
+  )
+1:zsystem flock invalid time arguments
+?(eval):zsystem:2: flock: invalid timeout
+?(eval):zsystem:3: flock: invalid timeout
+?(eval):zsystem:4: flock: invalid interval
+?(eval):zsystem:5: flock: invalid interval


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-29 20:35 [PATCH] Enable sub-second timeout in zsystem flock 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
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

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git