Re: wait_reading_process_ouput hangs in certain cases (w/ patches)

[Matthias Dahl <ml_emacs-lists@binary-island.eu>]

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

Hello Eli...

Attached you will find the revised patches which now properly
set the number of bytes read.

Sorry for the delay, but real life intervened.

Turns out, things were _a lot_ easier than I thought since the
C standard actually does have well-defined rules for computation
on unsigned types, so that no overflow happens. And since it is
modular arithmetic, it perfectly fits our use-case. :-) Sometimes
the simplest solution is right in front of your eyes and you are
thinking way too complicated.

So sorry for the trouble. Had I known this earlier, I would not
have skipped that part -- even though I had the best intentions,
obviously.

On with the rest of your mail...

On 07/11/17 17:40, Eli Zaretskii wrote:

> If we want, we can make it EMACS_INT, which will give us as much as
> Emacs can gobble.

Thanks for the suggestion, I changed it to EMACS_UINT -- we need an
unsigned type.

> I had my
> share of writing code based on what I read in the function commentary
> and some general common sense and familiarity with the internals, only
> to find out later that they were incomplete or even prone to wrong
> interpretation.  I'd like to minimize such occurrences as much as I
> can.

I guess I was overly idealistic. Sorry for that.

Nevertheless, it would be nice to improve the situation and make sure
that future changes to the codebase also take the commentaries into
account... and put those themselves to the same high standard as the
implementation itself. Just my two cents...

> But that's exactly the problem: we _don't_have_ documented interfaces
> on the C level, at least not of the quality we would like to have.  Do
> you really think that the commentary at the beginning of
> wait_reading_process_output describes what the function does anywhere
> close to completeness?  Far from it.

I know. OTOH, wait_reading_process_output might be a bad example as it
really could benefit greatly from a refactor since it is way too big,
complex and unclear. And describing its function properly and concisely
could prove quite difficult.

But generally, I pretty much agree on all that you said in the rest of
your mail.

> I'd actually say that the commentary is incomplete because it doesn't
> say what that "positive" value means.  Returning an opaque value from
> a function is not useful, unless you can pass it later to the same
> function to get it to do something special.

I agree. It also leads the user to make assumptions about the value
that might or might not turn out true for all cases... without properly
reading the code.

Thanks again for all your great feedback and review. I hope we now have
something that can be applied to the master branch. If anything comes up
in terms of bugs or problems that might be related, please poke me if I
miss it on the list and I will (try to) fix it.

Have a nice weekend,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-process-output-read-accounting.patch --]
[-- Type: text/x-patch; name="0001-Add-process-output-read-accounting.patch", Size: 1622 bytes --]

From 76b9697bf53c151849face5f95f2b07c2d0b3511 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Tue, 24 Oct 2017 15:55:53 +0200
Subject: [PATCH 1/2] Add process output read accounting

This tracks the bytes read from a process's stdin which is not used
anywhere yet but required for follow-up work.
* src/process.c (read_process_output): Track bytes read from a process.
* src/process.h (struct Lisp_Process): Add infd_num_bytes_read
to track bytes read from a process.
---
 src/process.c | 2 ++
 src/process.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/src/process.c b/src/process.c
index fc46e74332..904ca60863 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5900,6 +5900,8 @@ read_process_output (Lisp_Object proc, int channel)
   /* Now set NBYTES how many bytes we must decode.  */
   nbytes += carryover;
 
+  p->infd_num_bytes_read += nbytes;
+
   odeactivate = Vdeactivate_mark;
   /* There's no good reason to let process filters change the current
      buffer, and many callers of accept-process-output, sit-for, and
diff --git a/src/process.h b/src/process.h
index 5a044f669f..d49be55f10 100644
--- a/src/process.h
+++ b/src/process.h
@@ -129,6 +129,8 @@ struct Lisp_Process
     pid_t pid;
     /* Descriptor by which we read from this process.  */
     int infd;
+    /* Byte-count for process output read from `infd'.  */
+    EMACS_UINT infd_num_bytes_read;
     /* Descriptor by which we write to this process.  */
     int outfd;
     /* Descriptors that were created for this process and that need
-- 
2.15.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-src-process.c-wait_reading_process_output-Fix-wait_p.patch --]
[-- Type: text/x-patch; name="0002-src-process.c-wait_reading_process_output-Fix-wait_p.patch", Size: 3883 bytes --]

From a309cc76c93826c6ac7d51bba88900738910e504 Mon Sep 17 00:00:00 2001
From: Matthias Dahl <matthias.dahl@binary-island.eu>
Date: Tue, 24 Oct 2017 15:56:47 +0200
Subject: [PATCH 2/2] * src/process.c (wait_reading_process_output): Fix
 wait_proc hang.

If called recursively (through timers or process filters by the means
of accept-process-output), it is possible that the output of wait_proc
has already been read by one of those recursive calls, leaving the
original call hanging forever if no further output arrives through
that fd and no timeout has been specified. Implement proper checks by
taking advantage of the process output read accounting.
---
 src/process.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/src/process.c b/src/process.c
index 904ca60863..d4e152eb1c 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5003,6 +5003,8 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
   struct timespec got_output_end_time = invalid_timespec ();
   enum { MINIMUM = -1, TIMEOUT, INFINITY } wait;
   int got_some_output = -1;
+  EMACS_UINT initial_wait_proc_num_bytes_read = (wait_proc) ?
+                                                wait_proc->infd_num_bytes_read : 0;
 #if defined HAVE_GETADDRINFO_A || defined HAVE_GNUTLS
   bool retry_for_async;
 #endif
@@ -5161,6 +5163,20 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 	      && requeued_events_pending_p ())
 	    break;
 
+          /* Timers could have called `accept-process-output', thus reading the output
+             of wait_proc while we (in the worst case) wait endlessly for it to become
+             available later. So we need to check if data has been read and break out
+             early if that is so since our job has been fulfilled. */
+          if (wait_proc
+              && wait_proc->infd_num_bytes_read != initial_wait_proc_num_bytes_read)
+            {
+              /* Computations on unsigned types are well defined and won't overflow,
+                 so this is safe even if our initial value > our current value, in
+                 case of a wrap around. (ISO/IEC 9899:1999 §6.2.5/9) */
+              got_some_output = wait_proc->infd_num_bytes_read
+                                - initial_wait_proc_num_bytes_read;
+            }
+
           /* This is so a breakpoint can be put here.  */
           if (!timespec_valid_p (timer_delay))
               wait_reading_process_output_1 ();
@@ -5606,7 +5622,21 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 		 buffered-ahead character if we have one.  */
 
 	      nread = read_process_output (proc, channel);
-	      if ((!wait_proc || wait_proc == XPROCESS (proc))
+
+              /* In case a filter was run that called `accept-process-output', it is
+                 possible that the output from wait_proc was already read, leaving us
+                 waiting for it endlessly (if no timeout was specified). Thus, we need
+                 to check if data was already read. */
+              if (wait_proc
+                  && wait_proc->infd_num_bytes_read != initial_wait_proc_num_bytes_read)
+                {
+                  /* Computations on unsigned types are well defined and won't overflow,
+                     so this is safe even if our initial value > our current value, in
+                     case of a wrap around. (ISO/IEC 9899:1999 §6.2.5/9) */
+                  got_some_output = wait_proc->infd_num_bytes_read
+                                    - initial_wait_proc_num_bytes_read;
+                }
+	      else if ((!wait_proc || wait_proc == XPROCESS (proc))
 		  && got_some_output < nread)
 		got_some_output = nread;
 	      if (nread > 0)
-- 
2.15.0