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: 315 bytes --]

Hello Eli and Paul,

attached you find the updated patches which have all the discussed
changes and fixes.

If there is anything else, please let me know.

Thanks again for all the patience and valuable feedback.

Have a nice day,
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: 1642 bytes --]

From 94cd9ac3867305184f310dbf411729c59897c2c5 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 | 4 ++++
 src/process.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/src/process.c b/src/process.c
index fc46e74332..ab023457bd 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5886,6 +5886,10 @@ read_process_output (Lisp_Object proc, int channel)
       nbytes += buffered && nbytes <= 0;
     }
 
+  /* Don't count carryover as those bytes have already been count by
+     a previous iteration.  */
+  p->infd_num_bytes_read += nbytes;
+
   p->decoding_carryover = 0;
 
   /* At this point, NBYTES holds number of bytes just received
diff --git a/src/process.h b/src/process.h
index 5670f44736..96c19fcf81 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 modulo (UINTMAX_MAX + 1) for process output read from `infd'.  */
+    uintmax_t 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: 3769 bytes --]

From 1bbe69611bb4db8bd4149d57cfa5be548ee64c9d 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 | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/process.c b/src/process.c
index ab023457bd..b75ac171a1 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;
+  uintmax_t 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,19 @@ 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)
+            {
+              /* Make sure we don't overflow signed got_some_output.
+                 Calculating bytes read is modulo (UINTMAX_MAX + 1) and won't overflow. */
+              got_some_output = min(INT_MAX, (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 +5621,20 @@ 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)
+                {
+                  /* Make sure we don't overflow signed got_some_output.
+                     Calculating bytes read is modulo (UINTMAX_MAX + 1) and won't overflow. */
+                  got_some_output = min(INT_MAX, (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