Re: --wait interrupted on SIGWINCH; nanosleep not used

[Felix Dietrich]

Hello Tim,

Tim Rühsen <tim.ruehsen@gmx.de> wrote:

> On 25.01.21 17:28, Felix Dietrich wrote:
>> Hello,
>> I noticed that wget does interrupt its --wait between retrieval of
>> multiple files when my terminal window gets resized.

>> the “xsleep” function (in utils.c) only supports the continuation of
>> interrupted sleeps when the system provides a nanosleep function and
>> the appropriate code is enabled when HAVE_NANOSLEEP is defined – but
>> HAVE_NANOSLEEP does not get defined anywhere.

> Can you test that e.g. by adding
>   #define HAVE_NANOSLEEP 1
> to src/config.h and then rebuild with
>   make clean && make
> ?

> If you confirm the feature working again when built with the above
> instructions, I am going to fix the code appropriately.

yes, built with “#define HAVE_NANOSLEEP 1” added to “src/config.h” wget
does not start the next download prematurely when I resize the terminal
window.

>>  From here I am uncertain how to continue:
>>
>>    - Has the fallback code to “usleep” and select become obsolete and
>>      should simply be removed from “xselect” as Gnulib takes care of the
>>      compatibility?  (In this case the members of “struct timespec
>>      remaining”, probably, need to be initialised to 0 and checked before
>>      restarting nanosleep: the Gnulib’s fallback implementation does not
>>      appear to use or set it when interrupted.)

> The mentioned commit [1] should have removed the fallback code and the
> '#ifdef HAVE_NANOSLEEP'.

>> [1] Commit: a384f5e2e9afd11e363d011b474c2e5da5573103

If all that is necessary is the removal of all the preprocessor branches
from xsleep() except the nanosleep() one, I have appended a trivial
patch that does that; I may have overdone it on the prose/comments,
though.

I have moved a comment from the usleep() branch of xsleep() in
“src/utils.c” to “src/mswindows.c” as it was referenced there, but do
not actually understand it’s relevance to potential usleep()
implementations on Windows, or why they are used instead of Windows’
Sleep() or SleepEx().  At any rate: maybe the version of xsleep() in
“src/mswindows.c” should be merged into the one in “src/utils.c” after
the latter has been simplified to just using nanosleep()?

As an aside: Does the list allow MIME attachments (logs, patches)?
Which format is preferred?

-- 
Felix Dietrich


-- >8 --
Subject: [PATCH] src/utils.c: Remove obsolete code from xsleep()

This commit removes the preprocessor conditions in xsleep() leaving only
the nanosleep() branch.  Their removal was missed in commit
a384f5e2e9afd11e363d011b474c2e5da5573103 when Gnulib's nanosleep module
was added to provide fallback support.  This caused a bug where
nanosleep(), necessary for handling interruption caused by signals, was
not used anymore as the mentioned commit removed the setting of the
preprocessor definition HAVE_NANOSLEEP.
---
 src/mswindows.c |  4 +++-
 src/utils.c     | 43 +++++++++++--------------------------------
 2 files changed, 14 insertions(+), 33 deletions(-)

diff --git a/src/mswindows.c b/src/mswindows.c
index 87796ecf..751886c8 100644
--- a/src/mswindows.c
+++ b/src/mswindows.c
@@ -60,7 +60,9 @@ xsleep (double seconds)
 #if defined(HAVE_USLEEP) && defined(HAVE_SLEEP)
   if (seconds >= 1)
     {
-      /* Explained in utils.c. */
+      /* On some systems, usleep cannot handle values larger than
+         1,000,000.  If the period is larger than that, use sleep
+         first, then add usleep for subsecond accuracy.  */
       sleep (seconds);
       seconds -= (long) seconds;
     }
diff --git a/src/utils.c b/src/utils.c
index 426cda31..8d98b34f 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -2200,44 +2200,23 @@ run_with_timeout (double timeout, void (*fun) (void *), 
void *arg)
 void
 xsleep (double seconds)
 {
-#ifdef HAVE_NANOSLEEP
-  /* nanosleep is the preferred interface because it offers high
-     accuracy and, more importantly, because it allows us to reliably
-     restart receiving a signal such as SIGWINCH.  (There was an
-     actual Debian bug report about --limit-rate malfunctioning while
-     the terminal was being resized.)  */
-  struct timespec sleep, remaining;
+  /* Make sure REMAINING is initialised to 0: currently Gnulib’s fallback
+     implementation does not support the second argument of nanosleep()
+     and leaves it untouched; without proper initialisation this may
+     cause an infinite loop.
+
+     On systems that do not provide nanosleep() the unsupported second
+     argument to the fallback nanosleep() will cause xsleep() to return
+     prematurely and therefore features depending on it (like --wait and
+     --limit-rate) to malfunction when signals (such as SIGWINCH) are
+     received. */
+  struct timespec sleep, remaining = { 0 };
   sleep.tv_sec = (long) seconds;
   sleep.tv_nsec = 1000000000 * (seconds - (long) seconds);
   while (nanosleep (&sleep, &remaining) < 0 && errno == EINTR)
     /* If nanosleep has been interrupted by a signal, adjust the
        sleeping period and return to sleep.  */
     sleep = remaining;
-#elif defined(HAVE_USLEEP)
-  /* If usleep is available, use it in preference to select.  */
-  if (seconds >= 1)
-    {
-      /* On some systems, usleep cannot handle values larger than
-         1,000,000.  If the period is larger than that, use sleep
-         first, then add usleep for subsecond accuracy.  */
-      sleep (seconds);
-      seconds -= (long) seconds;
-    }
-  usleep (seconds * 1000000);
-#else /* fall back select */
-  /* Note that, although Windows supports select, it can't be used to
-     implement sleeping because Winsock's select doesn't implement
-     timeout when it is passed NULL pointers for all fd sets.  (But it
-     does under Cygwin, which implements Unix-compatible select.)  */
-  struct timeval sleep;
-  sleep.tv_sec = (long) seconds;
-  sleep.tv_usec = 1000000 * (seconds - (long) seconds);
-  select (0, NULL, NULL, NULL, &sleep);
-  /* If select returns -1 and errno is EINTR, it means we were
-     interrupted by a signal.  But without knowing how long we've
-     actually slept, we can't return to sleep.  Using gettimeofday to
-     track sleeps is slow and unreliable due to clock skew.  */
-#endif
 }
 
 #endif /* not WINDOWS */
-- 
2.30.0