GNU bug report logs - #20978
25.0.50; [PATCH 0/7] Emacs can return too fast when reading from any processes

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20978 in the body.
You can then email your comments to 20978 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 05:34:33 -0700

In GNU Emacs 25.0.50.63 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2015-07-04
Repository revision: 5c36788e76b22587e554960ed837f724473597a9
Windowing system distributor `The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04.2 LTS

Configured using:
 `configure 'CFLAGS=-O0 -g3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11


Overview:

The patch for debbugs:17647 causes emacs to return too fast sometimes
when reading from any processes. It made adaptive read buffering be
mostly dead code and caused readline-complete package to break. Instead,
limit timeout less aggressively.

Reproducing the problem:

emacs -Q -l repro.el

;; repro.el
(progn
  (setq explicit-bash-args '("--norc"))
  (setenv "EMACS" "")
  (setq comint-process-echoes t)
  (shell)
  (let* ((proc (get-buffer-process (current-buffer)))
         (filter-orig (process-filter proc))
         accumulated-output)
    (process-send-string proc "stty echo\n")
    (set-process-query-on-exit-flag proc nil)
    (dotimes (unused 10)
      (sleep-for .05))
    (set-process-filter proc (lambda (proc string) (push (list string) accumulated-output)))
    (process-send-string proc "echo a bit of text for testing\n")
    (dotimes (unused 100)
      (sleep-for .03))
    (message "waiting for any process: %s" (reverse accumulated-output))
    (switch-to-buffer "*Messages*")
    nil))


Output without my patches:

waiting for any process:
((ech) (o a ) (bi) (t ) (o) (f ) (t) (e) (x) (t) ( ) (f) (o) (r) ( ) (t) (e) (s) (t) (i) (n) (g) (
) (a bit of text for testing
) (bash-4.3$ ))

Output with my patches (also the same same output as before the
debbugs:17647 patch, aka git 05d2821):

waiting for any process:
((ec) (ho a bit of text for testing
a bit of text for testing
bash-4.3$ ))


Background:

The 17647 patch had a few things bundled in and the root problem and fix
was not documented. The problem was that we got output, and we
subsequently got a pselect of 0, but we have set
timeout_reduced_for_timers, so we don't break when we should. This is
the relevant condition:

if ((time_limit || nsecs) && nfds == 0 && ! timeout_reduced_for_timers)
  break;

This was fixed by setting the timeout to 0 if we've attempted to read
process output. However, that was too big a hammer.

What we actually want is what the original code tried to do, but had
some logic problems: read data, and stop if there is no more to read in
a reasonable amount of time or we've reached the requested timeout.

There is a reference to someone else having this issue as well, without
figuring out anything about it
http://lists.gnu.org/archive/html/bug-gnu-emacs/2014-06/msg00958.html
And there is a resulting workaround in flymake-tests.el.


About the patches:

The first 4 are refactoring and not necessarily dependent on any others,
but independent patches would conflict and they make the code clearer so
I included them. I can separate any of them if needed.

Patch 3 depends on the patch for debbugs:20976, which is a simple fix
and I expect no problem for it be applied first.

I was careful to not squash unrelated changes as the function is rather
complicated and I could have saved a fair amount of time if there
weren't unrelated changes in the patch that introduced this bug.

The last 2 patches are probably easier to understand squashed together,
but the last one one fixes a different avenue for the same bug to
manifest (SIGIO), which I haven't tried to reproduce, and includes more
circumstances (reading from a specific process and before we've gotten
any output), and I'm not sure if some code might depend on it's behavior
so I broke it out.

Make check succeeds on all the patches.

Ian Kelling (7):
  ; Minor cleanup of wait_reading_process_output
  ; Remove ADAPTIVE_READ_BUFFERING ifdef
  ; Rename local var to match function name
  ; Rename local var nsecs to adaptive_nsecs
  : Refactor timeouts in wait_reading_process_output
  Don't return as fast reading any process output
  Avoid returning early reading process output due to SIGIO

 src/process.c | 211 +++++++++++++++++++++++++++-------------------------------
 1 file changed, 98 insertions(+), 113 deletions(-)

--
2.4.5

Message #8 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: 20978 <at> debbugs.gnu.org
Subject: [PATCH 1/7] ; Minor cleanup of wait_reading_process_output
Date: Sat, 04 Jul 2015 05:37:34 -0700

* src/process.c (wait_reading_process_output): simplify logic, fix dos
version comments

diff --git a/src/process.c b/src/process.c
index 5272792..191f617 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4873,8 +4873,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
          no_avail = 1;
          FD_ZERO (&Available);
        }
-
-      if (!no_avail)
+      else
        {

 #ifdef ADAPTIVE_READ_BUFFERING
@@ -6965,9 +6964,7 @@ extern int sys_select (int, fd_set *, fd_set *, fd_set *,
    DO_DISPLAY means redisplay should be done to show subprocess
    output that arrives.

-   Return positive if we received input from WAIT_PROC (or from any
-   process if WAIT_PROC is null), zero if we attempted to receive
-   input but got none, and negative if we didn't even try.  */
+   Returns -1 signifying we got no output and did not try.  */

 int
 wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
--
2.4.5

Message #11 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: 20978 <at> debbugs.gnu.org
Subject: [PATCH 2/7] ; Remove ADAPTIVE_READ_BUFFERING ifdef
Date: Sat, 04 Jul 2015 05:40:07 -0700

* src/process.c (make-process, make-pipe-process, deactivate_process)
(wait_reading_process_output, read_process_output, send_process)
(init_process_emacs): ifdef ADAPTIVE_READ_BUFFERING was originally
added in case there was an operating system in which it was not
useful. That was 11 years ago and it hasn't happened. Make development
easier by not considering the effect of changes on a theoretical OS
where this is disabled.

diff --git a/src/process.c b/src/process.c
index 191f617..1541de2 100644
--- a/src/process.c
+++ b/src/process.c
@@ -224,11 +224,6 @@ static EMACS_INT update_tick;
 # define HAVE_SEQPACKET
 #endif

-#if !defined (ADAPTIVE_READ_BUFFERING) && !defined (NO_ADAPTIVE_READ_BUFFERING)
-#define ADAPTIVE_READ_BUFFERING
-#endif
-
-#ifdef ADAPTIVE_READ_BUFFERING
 #define READ_OUTPUT_DELAY_INCREMENT (TIMESPEC_RESOLUTION / 100)
 #define READ_OUTPUT_DELAY_MAX       (READ_OUTPUT_DELAY_INCREMENT * 5)
 #define READ_OUTPUT_DELAY_MAX_MAX   (READ_OUTPUT_DELAY_INCREMENT * 7)
@@ -242,10 +237,6 @@ static int process_output_delay_count;

 static bool process_output_skip;

-#else
-#define process_output_delay_count 0
-#endif
-
 static void create_process (Lisp_Object, char **, Lisp_Object);
 #ifdef USABLE_SIGIO
 static bool keyboard_bit_set (fd_set *);
@@ -1517,11 +1508,9 @@ usage: (make-process &rest ARGS)  */)
   pset_gnutls_cred_type (XPROCESS (proc), Qnil);
 #endif

-#ifdef ADAPTIVE_READ_BUFFERING
   XPROCESS (proc)->adaptive_read_buffering
     = (NILP (Vprocess_adaptive_read_buffering) ? 0
        : EQ (Vprocess_adaptive_read_buffering, Qt) ? 1 : 2);
-#endif

   /* Make the process marker point into the process buffer (if any).  */
   if (BUFFERP (buffer))
@@ -2184,11 +2173,9 @@ usage:  (make-pipe-process &rest ARGS)  */)
       FD_SET (inchannel, &input_wait_mask);
       FD_SET (inchannel, &non_keyboard_wait_mask);
     }
-#ifdef ADAPTIVE_READ_BUFFERING
   p->adaptive_read_buffering
     = (NILP (Vprocess_adaptive_read_buffering) ? 0
        : EQ (Vprocess_adaptive_read_buffering, Qt) ? 1 : 2);
-#endif

   /* Make the process marker point into the process buffer (if any).  */
   if (BUFFERP (buffer))
@@ -4183,7 +4170,6 @@ deactivate_process (Lisp_Object proc)
   emacs_gnutls_deinit (proc);
 #endif /* HAVE_GNUTLS */

-#ifdef ADAPTIVE_READ_BUFFERING
   if (p->read_output_delay > 0)
     {
       if (--process_output_delay_count < 0)
@@ -4191,7 +4177,6 @@ deactivate_process (Lisp_Object proc)
       p->read_output_delay = 0;
       p->read_output_skip = 0;
     }
-#endif

   /* Beware SIGCHLD hereabouts.  */

@@ -4876,7 +4861,6 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
       else
        {

-#ifdef ADAPTIVE_READ_BUFFERING
          /* Set the timeout for adaptive read buffering if any
             process has non-zero read_output_skip and non-zero
             read_output_delay, and we are not reading output for a
@@ -4908,7 +4892,6 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
              timeout = make_timespec (0, nsecs);
              process_output_skip = 0;
            }
-#endif

 #if defined (HAVE_NS)
           nfds = ns_select
@@ -5345,7 +5328,6 @@ read_process_output (Lisp_Object proc, int channel)
 #endif
        nbytes = emacs_read (channel, chars + carryover + buffered,
                             readmax - buffered);
-#ifdef ADAPTIVE_READ_BUFFERING
       if (nbytes > 0 && p->adaptive_read_buffering)
        {
          int delay = p->read_output_delay;
@@ -5371,7 +5353,6 @@ read_process_output (Lisp_Object proc, int channel)
              process_output_skip = 1;
            }
        }
-#endif
       nbytes += buffered;
       nbytes += buffered && nbytes <= 0;
     }
@@ -5840,7 +5821,6 @@ send_process (Lisp_Object proc, const char *buf, ptrdiff_t len,
 #endif
                written = emacs_write_sig (outfd, cur_buf, cur_len);
              rv = (written ? 0 : -1);
-#ifdef ADAPTIVE_READ_BUFFERING
              if (p->read_output_delay > 0
                  && p->adaptive_read_buffering == 1)
                {
@@ -5848,7 +5828,6 @@ send_process (Lisp_Object proc, const char *buf, ptrdiff_t len,
                  process_output_delay_count--;
                  p->read_output_skip = 0;
                }
-#endif
            }

          if (rv < 0)
@@ -7470,10 +7449,8 @@ init_process_emacs (void)
   num_pending_connects = 0;
 #endif

-#ifdef ADAPTIVE_READ_BUFFERING
   process_output_delay_count = 0;
   process_output_skip = 0;
-#endif

   /* Don't do this, it caused infinite select loops.  The display
      method should call add_keyboard_wait_descriptor on stdin if it
@@ -7638,7 +7615,6 @@ then a pipe is used in any case.
 The value takes effect when `start-process' is called.  */);
   Vprocess_connection_type = Qt;

-#ifdef ADAPTIVE_READ_BUFFERING
   DEFVAR_LISP ("process-adaptive-read-buffering", Vprocess_adaptive_read_buffering,
               doc: /* If non-nil, improve receive buffering by delaying after short reads.
 On some systems, when Emacs reads the output from a subprocess, the output data
@@ -7650,7 +7626,6 @@ If the value is t, the delay is reset after each write to the process; any other
 non-nil value means that the delay is not reset on write.
 The variable takes effect when `start-process' is called.  */);
   Vprocess_adaptive_read_buffering = Qt;
-#endif

   defsubr (&Sprocessp);
   defsubr (&Sget_process);
--
2.4.5

Message #14 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: 20978 <at> debbugs.gnu.org
Subject: [PATCH 3/7] ; Rename local var to match function name
Date: Sat, 04 Jul 2015 05:42:14 -0700

* src/process.c (wait_reading_process_output, status_notify):
Previously the function wait_reading_process_input was renamed to the
more logical wait_reading_process_output. Make it's local variables
consistent with that change.

diff --git a/src/process.c b/src/process.c
index 1541de2..d5569d4 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4586,7 +4586,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
   int xerrno;
   Lisp_Object proc;
   struct timespec timeout, end_time;
-  int got_some_input = -1;
+  int got_some_output = -1;
   ptrdiff_t count = SPECPDL_INDEX ();

   FD_ZERO (&Available);
@@ -4634,7 +4634,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
        break;

       /* After reading input, vacuum up any leftovers without waiting.  */
-      if (0 <= got_some_input)
+      if (0 <= got_some_output)
        nsecs = -1;

       /* Compute time from now till when time limit is up.  */
@@ -4755,7 +4755,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
              /* It's okay for us to do this and then continue with
                 the loop, since timeout has already been zeroed out.  */
              clear_waiting_for_input ();
-	      got_some_input = status_notify (NULL, wait_proc);
+	      got_some_output = status_notify (NULL, wait_proc);
              if (do_display) redisplay_preserve_echo_area (13);
            }
        }
@@ -4791,8 +4791,8 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
                    }
                  else
                    {
-		      if (got_some_input < nread)
-			got_some_input = nread;
+		      if (got_some_output < nread)
+			got_some_output = nread;
                      if (nread == 0)
                        break;
                      read_some_bytes = true;
@@ -5089,8 +5089,8 @@ 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)) && got_some_input < nread)
-		got_some_input = nread;
+	      if ((!wait_proc || wait_proc == XPROCESS (proc)) && got_some_output < nread)
+		got_some_output = nread;
              if (nread > 0)
                {
                  /* Since read_process_output can run a filter,
@@ -5250,7 +5250,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
       QUIT;
     }

-  return got_some_input;
+  return got_some_output;
 }
 
 /* Given a list (FUNCTION ARGS...), apply FUNCTION to the ARGS.  */
@@ -6659,7 +6659,7 @@ status_notify (struct Lisp_Process *deleting_process,
   Lisp_Object proc;
   Lisp_Object tail, msg;
   struct gcpro gcpro1, gcpro2;
-  int got_some_input = -1;
+  int got_some_output = -1;

   tail = Qnil;
   msg = Qnil;
@@ -6693,8 +6693,8 @@ status_notify (struct Lisp_Process *deleting_process,
            {
              int nread = read_process_output (proc, p->infd);
              if ((!wait_proc || wait_proc == XPROCESS (proc))
-                  && got_some_input < nread)
-		got_some_input = nread;
+                  && got_some_output < nread)
+               got_some_output = nread;
              if (nread <= 0)
                break;
            }
@@ -6729,7 +6729,7 @@ status_notify (struct Lisp_Process *deleting_process,

   update_mode_lines = 24;  /* In case buffers use %s in mode-line-format.  */
   UNGCPRO;
-  return got_some_input;
+  return got_some_output;
 }

 DEFUN ("internal-default-process-sentinel", Finternal_default_process_sentinel,
--
2.4.5

Message #17 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: 20978 <at> debbugs.gnu.org
Subject: [PATCH 4/7] ; Rename local var nsecs to adaptive_nsecs
Date: Sat, 04 Jul 2015 05:43:21 -0700

* src/process.c (wait_reading_process_output): Rename inner nsecs to
adaptive_nsecs. There is already an nsecs, and this function is
confusing enough.

diff --git a/src/process.c b/src/process.c
index d5569d4..68a815f 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4868,9 +4868,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
             Vprocess_adaptive_read_buffering is nil.  */
          if (process_output_skip && check_delay > 0)
            {
-	      int nsecs = timeout.tv_nsec;
-	      if (timeout.tv_sec > 0 || nsecs > READ_OUTPUT_DELAY_MAX)
-		nsecs = READ_OUTPUT_DELAY_MAX;
+	      int adaptive_nsecs = timeout.tv_nsec;
+              if (timeout.tv_sec > 0 || adaptive_nsecs > READ_OUTPUT_DELAY_MAX)
+		adaptive_nsecs = READ_OUTPUT_DELAY_MAX;
              for (channel = 0; check_delay > 0 && channel <= max_process_desc; channel++)
                {
                  proc = chan_process[channel];
@@ -4885,11 +4885,11 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
                        continue;
                      FD_CLR (channel, &Available);
                      XPROCESS (proc)->read_output_skip = 0;
-		      if (XPROCESS (proc)->read_output_delay < nsecs)
-			nsecs = XPROCESS (proc)->read_output_delay;
+		      if (XPROCESS (proc)->read_output_delay < adaptive_nsecs)
+			adaptive_nsecs = XPROCESS (proc)->read_output_delay;
                    }
                }
-	      timeout = make_timespec (0, nsecs);
+	      timeout = make_timespec (0, adaptive_nsecs);
              process_output_skip = 0;
            }

--
2.4.5

Message #20 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: 20978 <at> debbugs.gnu.org
Subject: [PATCH 5/7] : Refactor timeouts in wait_reading_process_output
Date: Sat, 04 Jul 2015 05:45:41 -0700

* src/process.c (wait_reading_process_output): Simplify timeouts with
an enum. Remove a redundant condition.

diff --git a/src/process.c b/src/process.c
index 68a815f..538455c 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4586,6 +4586,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
   int xerrno;
   Lisp_Object proc;
   struct timespec timeout, end_time;
+  enum { MINIMUM, TIMEOUT, INFINITY } wait;
   int got_some_output = -1;
   ptrdiff_t count = SPECPDL_INDEX ();

@@ -4601,21 +4602,19 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
                             waiting_for_user_input_p);
   waiting_for_user_input_p = read_kbd;

-  if (time_limit < 0)
-    {
-      time_limit = 0;
-      nsecs = -1;
-    }
-  else if (TYPE_MAXIMUM (time_t) < time_limit)
+  if (TYPE_MAXIMUM (time_t) < time_limit)
     time_limit = TYPE_MAXIMUM (time_t);

-  /* Since we may need to wait several times,
-     compute the absolute time to return at.  */
-  if (time_limit || nsecs > 0)
+  if (time_limit < 0 || nsecs < 0)
+    wait = MINIMUM;
+  else if (time_limit > 0 || nsecs > 0)
     {
-      timeout = make_timespec (time_limit, nsecs);
-      end_time = timespec_add (current_timespec (), timeout);
+      wait = TIMEOUT;
+      end_time = timespec_add (current_timespec (),
+                               make_timespec (time_limit, nsecs));
     }
+  else
+    wait = INFINITY;

   while (1)
     {
@@ -4635,19 +4634,15 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,

       /* After reading input, vacuum up any leftovers without waiting.  */
       if (0 <= got_some_output)
-	nsecs = -1;
+        wait = MINIMUM;

       /* Compute time from now till when time limit is up.  */
       /* Exit if already run out.  */
-      if (nsecs < 0)
+      if (wait == MINIMUM)
        {
-	  /* A negative timeout means
-	     gobble output available now
-	     but don't wait at all.  */
-
-	  timeout = make_timespec (0, 0);
+          timeout = make_timespec (0, 0);
        }
-      else if (time_limit || nsecs > 0)
+      else if (wait == TIMEOUT)
        {
          struct timespec now = current_timespec ();
          if (timespec_cmp (end_time, now) <= 0)
@@ -4698,24 +4693,20 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
              && requeued_events_pending_p ())
            break;

-	  /* A negative timeout means do not wait at all.  */
-	  if (nsecs >= 0)
-	    {
-	      if (timespec_valid_p (timer_delay))
-		{
-		  if (timespec_cmp (timer_delay, timeout) < 0)
-		    {
-		      timeout = timer_delay;
-		      timeout_reduced_for_timers = true;
-		    }
-		}
-	      else
-		{
-		  /* This is so a breakpoint can be put here.  */
-		  wait_reading_process_output_1 ();
-		}
-	    }
-	}
+          if (timespec_valid_p (timer_delay))
+            {
+              if (timespec_cmp (timer_delay, timeout) < 0)
+                {
+                  timeout = timer_delay;
+                  timeout_reduced_for_timers = true;
+                }
+            }
+          else
+            {
+              /* This is so a breakpoint can be put here.  */
+              wait_reading_process_output_1 ();
+            }
+        }

       /* Cause C-g and alarm signals to take immediate action,
         and cause input available signals to zero out timeout.
@@ -4964,7 +4955,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
       /*  If we woke up due to SIGWINCH, actually change size now.  */
       do_pending_window_change (0);

-      if ((time_limit || nsecs) && nfds == 0 && ! timeout_reduced_for_timers)
+      if (wait != INFINITY && nfds == 0 && ! timeout_reduced_for_timers)
        /* We waited the full specified time, so return now.  */
        break;
       if (nfds < 0)
@@ -6953,21 +6944,21 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 {
   register int nfds;
   struct timespec end_time, timeout;
+  enum { MINIMUM, TIMEOUT, INFINITY } wait;

-  if (time_limit < 0)
-    {
-      time_limit = 0;
-      nsecs = -1;
-    }
-  else if (TYPE_MAXIMUM (time_t) < time_limit)
+  if (TYPE_MAXIMUM (time_t) < time_limit)
     time_limit = TYPE_MAXIMUM (time_t);

-  /* What does time_limit really mean?  */
-  if (time_limit || nsecs > 0)
+  if (time_limit < 0 || nsecs < 0)
+    wait = MINIMUM;
+  else if (time_limit > 0 || nsecs > 0)
     {
-      timeout = make_timespec (time_limit, nsecs);
-      end_time = timespec_add (current_timespec (), timeout);
+      wait = TIMEOUT;
+      end_time = timespec_add (current_timespec (),
+                               make_timespec (time_limit, nsecs));
     }
+  else
+    wait = INFINITY;

   /* Turn off periodic alarms (in case they are in use)
      and then turn off any other atimers,
@@ -6993,15 +6984,11 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,

       /* Compute time from now till when time limit is up.  */
       /* Exit if already run out.  */
-      if (nsecs < 0)
+      if (wait == MINIMUM)
        {
-	  /* A negative timeout means
-	     gobble output available now
-	     but don't wait at all.  */
-
-	  timeout = make_timespec (0, 0);
+          timeout = make_timespec (0, 0);
        }
-      else if (time_limit || nsecs > 0)
+      else if (wait == TIMEOUT)
        {
          struct timespec now = current_timespec ();
          if (timespec_cmp (end_time, now) <= 0)
@@ -7039,7 +7026,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
              && requeued_events_pending_p ())
            break;

-	  if (timespec_valid_p (timer_delay) && nsecs >= 0)
+	  if (timespec_valid_p (timer_delay))
            {
              if (timespec_cmp (timer_delay, timeout) < 0)
                {
@@ -7083,7 +7070,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
       /*  If we woke up due to SIGWINCH, actually change size now.  */
       do_pending_window_change (0);

-      if ((time_limit || nsecs) && nfds == 0 && ! timeout_reduced_for_timers)
+      if (wait != INFINITY && nfds == 0 && ! timeout_reduced_for_timers)
        /* We waited the full specified time, so return now.  */
        break;

--
2.4.5

Message #23 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: 20978 <at> debbugs.gnu.org
Subject: [PATCH 6/7] Don't return as fast reading any process output
Date: Sat, 04 Jul 2015 05:47:07 -0700

* src/process.c (wait_reading_process_output): The patch for
debbugs:17647 returns too fast sometimes when reading from any
processes. Revert part of it, and limit the timeout more
sensibly.

diff --git a/src/process.c b/src/process.c
index 538455c..cb48e5f 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4585,7 +4585,8 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
   bool no_avail;
   int xerrno;
   Lisp_Object proc;
-  struct timespec timeout, end_time;
+  struct timespec timeout, end_time, timer_delay;
+  struct timespec got_output_end_time = invalid_timespec ();
   enum { MINIMUM, TIMEOUT, INFINITY } wait;
   int got_some_output = -1;
   ptrdiff_t count = SPECPDL_INDEX ();
@@ -4618,7 +4619,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,

   while (1)
     {
-      bool timeout_reduced_for_timers = false;
+      bool process_skipped = false;

       /* If calling from keyboard input, do not quit
         since we want to return C-g as an input character.
@@ -4632,10 +4633,6 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
       if (! NILP (wait_for_cell) && ! NILP (XCAR (wait_for_cell)))
        break;

-      /* After reading input, vacuum up any leftovers without waiting.  */
-      if (0 <= got_some_output)
-        wait = MINIMUM;
-
       /* Compute time from now till when time limit is up.  */
       /* Exit if already run out.  */
       if (wait == MINIMUM)
@@ -4661,8 +4658,6 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
       if (NILP (wait_for_cell)
          && just_wait_proc >= 0)
        {
-	  struct timespec timer_delay;
-
          do
            {
              unsigned old_timers_run = timers_run;
@@ -4693,19 +4688,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
              && requeued_events_pending_p ())
            break;

-          if (timespec_valid_p (timer_delay))
-            {
-              if (timespec_cmp (timer_delay, timeout) < 0)
-                {
-                  timeout = timer_delay;
-                  timeout_reduced_for_timers = true;
-                }
-            }
-          else
-            {
-              /* This is so a breakpoint can be put here.  */
+          /* This is so a breakpoint can be put here.  */
+          if (!timespec_valid_p (timer_delay))
               wait_reading_process_output_1 ();
-            }
         }

       /* Cause C-g and alarm signals to take immediate action,
@@ -4875,6 +4860,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
                      if (!XPROCESS (proc)->read_output_skip)
                        continue;
                      FD_CLR (channel, &Available);
+                      process_skipped = true;
                      XPROCESS (proc)->read_output_skip = 0;
                      if (XPROCESS (proc)->read_output_delay < adaptive_nsecs)
                        adaptive_nsecs = XPROCESS (proc)->read_output_delay;
@@ -4884,6 +4870,30 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
              process_output_skip = 0;
            }

+          /* If we've got some output and haven't limited our timeout
+           * with adaptive read buffering, limit it. */
+          if (got_some_output > 0 && !process_skipped
+              && (timeout.tv_sec
+                  || timeout.tv_nsec > READ_OUTPUT_DELAY_INCREMENT))
+            timeout = make_timespec (0, READ_OUTPUT_DELAY_INCREMENT);
+
+
+          if (NILP (wait_for_cell) && just_wait_proc >= 0
+              && timespec_valid_p (timer_delay)
+              && timespec_cmp (timer_delay, timeout) < 0)
+            {
+              struct timespec timeout_abs = timespec_add (current_timespec (),
+                                                          timeout);
+              if (!timespec_valid_p (got_output_end_time)
+                  || timespec_cmp (timeout_abs,
+                                   got_output_end_time) < 0)
+                got_output_end_time = timeout_abs;
+              timeout = timer_delay;
+            }
+          else
+            got_output_end_time = invalid_timespec ();
+
+
 #if defined (HAVE_NS)
           nfds = ns_select
 #elif defined (HAVE_GLIB)
@@ -4955,9 +4965,17 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
       /*  If we woke up due to SIGWINCH, actually change size now.  */
       do_pending_window_change (0);

-      if (wait != INFINITY && nfds == 0 && ! timeout_reduced_for_timers)
-	/* We waited the full specified time, so return now.  */
-	break;
+      if (nfds == 0)
+        {
+          struct timespec now = current_timespec ();
+          if ((timeout.tv_sec == 0 && timeout.tv_nsec == 0)
+              || (wait == TIMEOUT && timespec_cmp (end_time, now) <= 0)
+              || (!process_skipped && got_some_output > 0
+                  && (!timespec_valid_p (got_output_end_time)
+                      || timespec_cmp (got_output_end_time, now) <= 0)))
+            break;
+        }
+
       if (nfds < 0)
        {
          if (xerrno == EINTR)
@@ -5084,6 +5102,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
                got_some_output = nread;
              if (nread > 0)
                {
+                  /* vacuum up any leftovers without waiting.  */
+                  if (wait_proc == XPROCESS (proc))
+                    wait = MINIMUM;
                  /* Since read_process_output can run a filter,
                     which can call accept-process-output,
                     don't try to read from any other processes
--
2.4.5

Message #26 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: 20978 <at> debbugs.gnu.org
Subject: [PATCH 7/7] Avoid returning early reading process output due to SIGIO
Date: Sat, 04 Jul 2015 05:48:11 -0700

* src/process.c (wait_reading_process_output): Extend the behavior of
not breaking due to not finding output when a timer has lowered the
timeout to include when SIGIO lowers the timeout.

diff --git a/src/process.c b/src/process.c
index 052f94e..f64a064 100644
--- a/src/process.c
+++ b/src/process.c
@@ -4967,12 +4967,17 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,

       if (nfds == 0)
         {
+          /* If we've past the requested timeout, or got some output
+             and aren't skipping processes, and haven't lowered our
+             timeout due to timers or SIGIO, or have waited a long
+             amount of time due to repeated timers */
           struct timespec now = current_timespec ();
-          if ((timeout.tv_sec == 0 && timeout.tv_nsec == 0)
+          if (wait == MINIMUM
               || (wait == TIMEOUT && timespec_cmp (end_time, now) <= 0)
               || (!process_skipped && got_some_output > 0
                   && (!timespec_valid_p (got_output_end_time)
-                      || timespec_cmp (got_output_end_time, now) <= 0)))
+                      || timespec_cmp (got_output_end_time, now) <= 0)
+                  && (timeout.tv_sec > 0 || timeout.tv_nsec > 0)))
             break;
         }

--
2.4.5

Message #29 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ian Kelling <ian <at> iankelling.org>
Cc: 20978 <at> debbugs.gnu.org
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 15:52:44 +0300

> From: Ian Kelling <ian <at> iankelling.org>
> Date: Sat, 04 Jul 2015 05:34:33 -0700
> 
> About the patches:
> 
> The first 4 are refactoring and not necessarily dependent on any others,
> but independent patches would conflict and they make the code clearer so
> I included them. I can separate any of them if needed.
> 
> Patch 3 depends on the patch for debbugs:20976, which is a simple fix
> and I expect no problem for it be applied first.

Thanks.

I don't know how others feel, but I personally would prefer that
patches not be split between different messages, as that makes it
harder to grasp and apply.  Especially since some (most) of them
belong to the same changeset.  And refactoring should IMO be together
with the actual changes.

> I was careful to not squash unrelated changes as the function is rather
> complicated and I could have saved a fair amount of time if there
> weren't unrelated changes in the patch that introduced this bug.

Unrelated is in the eyes of the beholder.  We all have our different
views on that.

Thanks again for working on this.  I will defer to Paul to provide the
_real_ feedback ;-)

Message #32 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 20978 <at> debbugs.gnu.org
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 06:13:37 -0700

Eli Zaretskii <eliz <at> gnu.org> writes:
>
> I don't know how others feel, but I personally would prefer that
> patches not be split between different messages, as that makes it
> harder to grasp and apply.  Especially since some (most) of them
> belong to the same changeset.

That was my first intuition too. I hadn't submitted a set of patches
before to emacs and I couldn't find any documentation on it, except git
format-patch docs seems to assume you would always use separate
messages. Say the word and I will send one more message which is all of
them together.

> And refactoring should IMO be together
> with the actual changes.
>

Imo, it depends.

>> I was careful to not squash unrelated changes as the function is rather
>> complicated and I could have saved a fair amount of time if there
>> weren't unrelated changes in the patch that introduced this bug.
>
> Unrelated is in the eyes of the beholder.  We all have our different
> views on that.

Agreed.

Message #35 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Ian Kelling <ian <at> iankelling.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 20978 <at> debbugs.gnu.org
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 06:22:25 -0700

Eli Zaretskii <eliz <at> gnu.org> writes:

> Thanks again for working on this.  I will defer to Paul to provide the
> _real_ feedback ;-)

Yw. Thanks for the encouragement.

Message #38 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ian Kelling <ian <at> iankelling.org>
Cc: 20978 <at> debbugs.gnu.org
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 16:25:31 +0300

> From: Ian Kelling <ian <at> iankelling.org>
> Cc: 20978 <at> debbugs.gnu.org
> Date: Sat, 04 Jul 2015 06:13:37 -0700
> 
> Say the word and I will send one more message which is all of them
> together.

Please wait for others to chime in, I'd hate to have you do
unnecessary work, especially as I'm not the one to review your
patches.

Thanks.

Message #41 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 20978 <at> debbugs.gnu.org, Ian Kelling <ian <at> iankelling.org>
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 14:56:00 -0400

>> Say the word and I will send one more message which is all of them
>> together.

I don't think it's worth resending, no.

> Please wait for others to chime in, I'd hate to have you do
> unnecessary work, especially as I'm not the one to review
> your patches.

It's a "standard" work flow in the Git world.  Presumably we should have
a Gnus command which (after marking the 7 messages) checks out a fresh
branch, and applies those 7 patches, after which you can look at the
separately or together at your leisure with Git/VC/Magit/...


        Stefan

Message #44 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 20978 <at> debbugs.gnu.org, ian <at> iankelling.org
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 22:30:25 +0300

> From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
> Cc: Ian Kelling <ian <at> iankelling.org>, 20978 <at> debbugs.gnu.org
> Date: Sat, 04 Jul 2015 14:56:00 -0400
> 
> It's a "standard" work flow in the Git world.

I know.  But it doesn't mean I must like it, nor that we must adopt
it.

Message #47 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 20978 <at> debbugs.gnu.org, ian <at> iankelling.org
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Sat, 04 Jul 2015 18:20:37 -0400

>> It's a "standard" work flow in the Git world.
> I know.  But it doesn't mean I must like it, nor that we must
> adopt it.

No, indeed, but it's worth taking a good look at it, since the Linux
guys have spent a fair bit of effort to streamline their process, to
deal with the large number of patches they handle.


        Stefan

Message #52 received at 20978-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ian Kelling <ian <at> iankelling.org>
Cc: 20978-done <at> debbugs.gnu.org
Subject: Re: 25.0.50; [PATCH 0/7] Emacs can return too fast when reading from
 any processes
Date: Sun, 05 Jul 2015 19:28:54 -0700

[Message part 1 (text/plain, inline)]

Thanks for the patches; they were very clear and organized, in a difficult code 
area.  I installed them into the Emacs master with minor changes, along with the 
attached further patch, and am marking this bug as done.

As far as patch format goes, I mildly prefer attachments but the form you used 
was fine.  My preference is because I am not on the bug-gnu-emacs mailing list, 
and don't know how to retrieve your email from debbugs.gnu.org or from 
lists.gnu.org -- the best I could come up with was the mbox format of 
lists.gnu.org but that did not apply cleanly.

[0008-Avoid-duplicate-calls-to-current_timespec.patch (text/x-diff, attachment)]

Message #55 received at 20978 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: 20978 <at> debbugs.gnu.org
Cc: eggert <at> cs.ucla.edu, ian <at> iankelling.org
Subject: Re: bug#20978: 25.0.50;
 [PATCH 0/7] Emacs can return too fast when reading from any processes
Date: Mon, 06 Jul 2015 02:44:56 -0400

Paul Eggert wrote:

> As far as patch format goes, I mildly prefer attachments but the form
> you used was fine.  My preference is because I am not on the
> bug-gnu-emacs mailing list, and don't know how to retrieve your email
> from debbugs.gnu.org or from lists.gnu.org -- the best I could come up
> with was the mbox format of lists.gnu.org but that did not apply
> cleanly.

There's M-x gnus-read-ephemeral-emacs-bug-group, if that helps.
Or wget 'http://debbugs.gnu.org/cgi/bugreport.cgi?mbox=yes;bug=20978'.
(But I don't really get what the problem is.)

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.