bug#36591: 26.2; Term's pager seems broken

bug#36591: 26.2; Term's pager seems broken

[Adam Bliss]

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

Hello,

Recently I learned about the pager-mode built into M-x term. I tried
it with some excitement, but discovered that, while it works fine in
emacs 24, it seems completely broken in emacs 25-26.

To reproduce, simply open a terminal (M-x term); enable the pager (C-c
C-q); and run any command that outputs more than a screenful (like
`seq 100`). After paging to the end (with the spacebar), try to type
another command (like `echo foo`). The terminal appears to hang until
killed. (Actually, the inferior process is still running and receiving
your input, but the term buffer displays no further output. You can
force it to dump some output with C-c M-x term-continue subjob, but it
goes back to being hung after.)

I hacked up this little elisp function to demonstrate the bug:


```
(defun test-bug () "demonstrate bug" (interactive)
  (progn
    (term "/bin/bash")
    (sit-for 2)
    (term-pager-enable)
    (term-send-raw-string "seq 50\n")
    (sit-for 2)
    (term-pager-page 50)
    (term-send-raw-string "echo foo\n")
    (sit-for 2)
    (term-line-mode)
    (move-beginning-of-line nil)
    (forward-line -1)
    (let ((code  (if (= (string-to-char "f") (char-after)) 0 1)))
      (kill-emacs code)
      ;;(message "answer: %d" code)
      )
    )
  )
(test-bug)
```

Then, I wrote this little bash script for use in `git bisect`:

```
#!/bin/bash
set -euxo pipefail
date
git clean -dxf
./autogen.sh || exit 125
./configure --with-x=no --without-makeinfo --with-gnutls=yes || exit 125
make -j4 || exit 125
exec src/emacs -Q -l ../bisect.el
```

The bisect was a bit rougher than I expected, for three reasons: (1)
my dumb elisp has some kind of race condition, and sometimes will
neither pass nor fail but hang waiting for further input; (2) the
repo's autogen/autotools/Makefile don't seem to agree with `git
checkout` about timestamps, so I had pretty much had to do a clean
build rather than an incremental build at each step; (3) several
commits in the vicinity of the First Bad Commit have builds which are
broken without gnutls.

However, I was eventually able to get this output as the First Bad
Commit:

9755b75300b7c451bc79984eed2e346ce0a4ffb5 is the first bad commit
commit 9755b75300b7c451bc79984eed2e346ce0a4ffb5
Author: Lars Ingebrigtsen <larsi@gnus.org>
Date:   Tue Feb 16 13:37:33 2016 +1100

    Allow setting the filter masks later
    * src/process.c (Fset_process_filter): Don't set the socket
    masks here, because we may not have a socket yet.
    (set_process_filter_masks): New function.
    (connect_network_socket): Set the filter masks here.


I hope that this is helpful in tracking down the bug.

Please let me know if any further details are required.

Thanks,

--Adam

[-- Attachment #2: Type: text/html, Size: 3110 bytes --]

bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky]

Adam Bliss <abliss@gmail.com> writes:

> I hope that this is helpful in tracking down the bug.

Thanks for doing this, it was very helpful.  That commit looks like a
pretty innocent refactoring, but it actually reverses the sense of the
EQ (p->filter, Qt) check, because the pset_filter call is moved earlier.

So the bug can be fixed by adding a single '!'.  Eli, any chance of
having this fix in the release branch?


--- i/src/process.c
+++ w/src/process.c
@@ -1230,7 +1230,7 @@ set_process_filter_masks (struct Lisp_Process *p)
 {
   if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
     delete_read_fd (p->infd);
-  else if (EQ (p->filter, Qt)
+  else if (!EQ (p->filter, Qt)
 	   /* Network or serial process not stopped:  */
 	   && !EQ (p->command, Qt))
     add_process_read_fd (p->infd);

bug#36591: 26.2; Term's pager seems broken

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Tue, 23 Jul 2019 09:53:37 -0400
> Cc: 36591@debbugs.gnu.org
> 
> Adam Bliss <abliss@gmail.com> writes:
> 
> > I hope that this is helpful in tracking down the bug.
> 
> Thanks for doing this, it was very helpful.  That commit looks like a
> pretty innocent refactoring, but it actually reverses the sense of the
> EQ (p->filter, Qt) check, because the pset_filter call is moved earlier.
> 
> So the bug can be fixed by adding a single '!'.  Eli, any chance of
> having this fix in the release branch?

I don't think I understand why the change fixes the bug, sorry.  Can
you tell in more detail?  Also, the same function is called in another
place, so what will this change do to that other caller?

And how come we lived with this bug for 3 years without having noticed
it?

bug#36591: 26.2; Term's pager seems broken

[Adam Bliss]

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

On Tue, Jul 23, 2019, 13:40 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Noam Postavsky <npostavs@gmail.com>
> > Date: Tue, 23 Jul 2019 09:53:37 -0400
> > Cc: 36591@debbugs.gnu.org
> >
> > So the bug can be fixed by adding a single '!'.  Eli, any chance of
> > having this fix in the release branch?
>
> I don't think I understand why the change fixes the bug, sorry.  Can
> you tell in more detail?  Also, the same function is called in another
> place, so what will this change do to that other caller?
>
> And how come we lived with this bug for 3 years without having noticed
> it?
>

I don't understand it either, and I can't answer your questions; however I
can confirm that the proposed patch, atop my local build of 26.2, does seem
to fix the bug.

Thanks,
--Adam

>

[-- Attachment #2: Type: text/html, Size: 1538 bytes --]

bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky]

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

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think I understand why the change fixes the bug, sorry.  Can
> you tell in more detail?

Okay.  First, see attached reduced test case which demonstrates the bug.


[-- Attachment #2: demo for bug --]
[-- Type: text/plain, Size: 1133 bytes --]

(with-current-buffer (get-buffer-create "*test buffer*")
  (let* ((oldproc (get-buffer-process (current-buffer))))
    (when oldproc
      (kill-process oldproc)
      (accept-process-output oldproc)))
  (erase-buffer)
  (display-buffer (current-buffer))
  (let* ((print-level nil)
         (print-length nil)
         (proc (start-process
                "test proc" (current-buffer)
                (concat invocation-directory invocation-name)
                "-Q" "--batch" "--eval"
                (prin1-to-string
                 '(let (s)
                    (while (setq s (read-from-minibuffer "$ "))
                      (princ s)
                      (princ "\n")))))))
    (send-string proc "one\n")
    (sit-for 0.5)                       ; Read "one".
    (set-process-filter proc t)         ; Stop reading from proc.
    (send-string proc "two\n")
    (sit-for 1)                         ; Can't read "two" yet.
    (set-process-filter
     proc                               ; Resume reading from proc.
     #'internal-default-process-filter)
    (sit-for 0.5)                       ; Read "two" from proc.
    ))

[-- Attachment #3: Type: text/plain, Size: 5144 bytes --]


In Emacs 25 it shows a buffer named "*test buffer*" with contents

    $ one
    $ two
    $ 

In Emacs 26, the "two" never gets read, so the result is

    $ one
    $ 

Calling (set-process-filter PROC t) is supposed to turn off reading for
PROC.  This works fine.  But (set-process-filter PROC FILTER) doesn't
turn on reading again in Emacs 26.  Neither of the cases in
set_process_filter_masks are taken in the case when FILTER is not t.

    DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
      ...
      (Lisp_Object process, Lisp_Object filter)
    {
      ...
      pset_filter (p, filter); // <-- sets p->filter = filter;

      if (p->infd >= 0)
        set_process_filter_masks (p);
      ...
    }

    static void
    set_process_filter_masks (struct Lisp_Process *p)
    {
      if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
        delete_read_fd (p->infd);
      else if (EQ (p->filter, Qt)
           /* Network or serial process not stopped:  */
           && !EQ (p->command, Qt))
        add_process_read_fd (p->infd);
    }

In Emacs 25 it looks like the 'if' cases are the same, but there is a
subtle difference: the first 'if' checks 'filter', while the second
checks 'p->filter'.  And furthermore note that pset_filter is called
after this check against 'p->filter', so it is checking the "original"
'p->filter' value (which means that Emacs 25 has a bug that the fd is
incorrectly enabled if setting the filter to t twice, i.e., (progn
(set-process-filter PROC t) (set-process-filter PROC t))).

    DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
      ...
      (register Lisp_Object process, Lisp_Object filter)
    {
      ...
      if (p->infd >= 0)
        {
          if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
            {
              FD_CLR (p->infd, &input_wait_mask);
              FD_CLR (p->infd, &non_keyboard_wait_mask);
            }
          else if (EQ (p->filter, Qt)
                   /* Network or serial process not stopped:  */
                   && !EQ (p->command, Qt))
            {
              FD_SET (p->infd, &input_wait_mask);
              FD_SET (p->infd, &non_keyboard_wait_mask);
            }
        }

      pset_filter (p, filter);

The patch found by Adam's bisect put the pset_filter call before this
check, so that Emacs 26 checks the 'p->filter' after it's been set
(i.e., the value of the 'filter' parameter).  So the second case is no
longer entered when calling (set-filter-process PROC FILTER).

> Also, the same function is called in another
> place, so what will this change do to that other caller?

Hmm, it's difficult to say, there are a lot of optional code paths.  I
suspect socket subprocesses might suffer from the same bug, but there
are also some (redundant?) calls add_process_read_fd that might cover it
up (notice that all calls are guarded by !EQ (p->filter, Qt), i.e., the
corrected version of the check in set_process_filter_masks).

    static void
    connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
                            Lisp_Object use_external_socket_p)
    {
      ...
      if (p->is_non_blocking_client)
        {...}
      else
        /* A server may have a client filter setting of Qt, but it must
           still listen for incoming connects unless it is stopped.  */
        if ((!EQ (p->filter, Qt) && !EQ (p->command, Qt))
            || (EQ (p->status, Qlisten) && NILP (p->command)))
          add_process_read_fd (inch);
      ...
    }
    ...
    static void
    server_accept_connection (Lisp_Object server, int channel)
    {
      ...
      /* Client processes for accepted connections are not stopped initially.  */
      if (!EQ (p->filter, Qt))
        add_process_read_fd (s);
      ...
    }
    ...
    int
    wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
                                 bool do_display,
                                 Lisp_Object wait_for_cell,
                                 struct Lisp_Process *wait_proc, int just_wait_proc)
    {
      ...
                      if (0 <= p->infd && !EQ (p->filter, Qt)
                          && !EQ (p->command, Qt))
                        add_process_read_fd (p->infd);
      ...
    }
    ...
    DEFUN ("continue-process", Fcontinue_process, Scontinue_process, 0, 2, 0,
           ...)
      (Lisp_Object process, Lisp_Object current_group)
    {
      ...
          if (EQ (p->command, Qt)
              && p->infd >= 0
              && (!EQ (p->filter, Qt) || EQ (p->status, Qlisten)))
            {
              add_process_read_fd (p->infd);


> And how come we lived with this bug for 3 years without having noticed
> it?

I think there is very little use of t as a process filter value.  Maybe
term.el is the only user of it.  As to why nobody noticed the problem in
term.el earlier, I have the impression that most users just assume
term.el will be buggy and don't even bother reporting problems with it
(typical suggestions are "run screen or tmux to handle escape codes
properly", or "use the libvterm Emacs extension instead").

bug#36591: 26.2; Term's pager seems broken

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: abliss@gmail.com,  36591@debbugs.gnu.org
> Date: Tue, 23 Jul 2019 22:07:24 -0400
> 
>     static void
>     set_process_filter_masks (struct Lisp_Process *p)
>     {
>       if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
>         delete_read_fd (p->infd);
>       else if (EQ (p->filter, Qt)
>            /* Network or serial process not stopped:  */
>            && !EQ (p->command, Qt))
>         add_process_read_fd (p->infd);
>     }
> 
> In Emacs 25 it looks like the 'if' cases are the same, but there is a
> subtle difference: the first 'if' checks 'filter', while the second
> checks 'p->filter'.  And furthermore note that pset_filter is called
> after this check against 'p->filter', so it is checking the "original"
> 'p->filter' value (which means that Emacs 25 has a bug that the fd is
> incorrectly enabled if setting the filter to t twice, i.e., (progn
> (set-process-filter PROC t) (set-process-filter PROC t))).
> 
>     DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
>       ...
>       (register Lisp_Object process, Lisp_Object filter)
>     {
>       ...
>       if (p->infd >= 0)
>         {
>           if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
>             {
>               FD_CLR (p->infd, &input_wait_mask);
>               FD_CLR (p->infd, &non_keyboard_wait_mask);
>             }
>           else if (EQ (p->filter, Qt)
>                    /* Network or serial process not stopped:  */
>                    && !EQ (p->command, Qt))
>             {
>               FD_SET (p->infd, &input_wait_mask);
>               FD_SET (p->infd, &non_keyboard_wait_mask);
>             }
>         }
> 
>       pset_filter (p, filter);

I see that it's a small mess, but I don't think I understand your
reasoning about setting the filter to t twice: it looks to me that
both calls will clear the bit of p->infd, because they both will
trigger this clause:

  current emacs-26:

      if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
        delete_read_fd (p->infd);

  previous code:

          if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
            {
              FD_CLR (p->infd, &input_wait_mask);
              FD_CLR (p->infd, &non_keyboard_wait_mask);
            }

Am I missing something?

> The patch found by Adam's bisect put the pset_filter call before this
> check, so that Emacs 26 checks the 'p->filter' after it's been set
> (i.e., the value of the 'filter' parameter).  So the second case is no
> longer entered when calling (set-filter-process PROC FILTER).

Right, this part is clear.  My problem was with the solution that just
reverses the 2nd test.  See below.

> > Also, the same function is called in another
> > place, so what will this change do to that other caller?
> 
> Hmm, it's difficult to say, there are a lot of optional code paths.  I
> suspect socket subprocesses might suffer from the same bug, but there
> are also some (redundant?) calls add_process_read_fd that might cover it
> up (notice that all calls are guarded by !EQ (p->filter, Qt), i.e., the
> corrected version of the check in set_process_filter_masks).

Yes, that's another small mess.

I think we need to be more careful here if we want to have this fixed
in Emacs 26.3.  The problem with your proposal is that it has
potentially far-reaching consequences:

  . if the filter is not t, we will now call add_process_read_fd every
    time, for no good reason
  . the patch changes how connect_network_socket works in ways that we
    don't sufficiently understand

I would like to leave connect_network_socket alone on the release
branch, and just fix the problem with set-process-filter.  I think the
easiest way is simply not to call set_process_filter_masks in
set-process-filter, and instead restore the way that code worked
before 9755b753, i.e. let it test the argument FILTER _before_ that
filter is installed.  Do you see any problems with this fix?

On the master branch we should clean up the confusing set of if
clauses, both in set-process-filter and in connect_network_socket.
Perhaps Lars could describe his reasoning for making the change which
introduced set_process_filter_masks and what problem it tried to
solve.  (Btw, the log message for that change seems to imply that
set-process-filter should not have called set_process_filter_masks,
something that the change itself disagrees with.  An omission?)

bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky]

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

Eli Zaretskii <eliz@gnu.org> writes:

> I see that it's a small mess, but I don't think I understand your
> reasoning about setting the filter to t twice: it looks to me that
> both calls will clear the bit of p->infd, because they both will
> trigger this clause:
>
>       if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))

> Am I missing something?

Argh, no, I was confused.  The code in Emacs 25 is correct, although
(IMO) somewhat misleading.

>   . if the filter is not t, we will now call add_process_read_fd every
>     time, for no good reason

(This is a moot point due to the second problem, but)
add_process_read_fd just sets some bits, which is harmless even if
repeated.

>   . the patch changes how connect_network_socket works in ways that we
>     don't sufficiently understand

Yes, I agree this is a serious problem.

> I would like to leave connect_network_socket alone on the release
> branch, and just fix the problem with set-process-filter.  I think the
> easiest way is simply not to call set_process_filter_masks in
> set-process-filter, and instead restore the way that code worked
> before 9755b753, i.e. let it test the argument FILTER _before_ that
> filter is installed.  Do you see any problems with this fix?

I think that makes sense, patch attached.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1719 bytes --]

From ee13774efa2ac4b288f339c668b6002e27fbbe8f Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 24 Jul 2019 20:33:18 -0400
Subject: [PATCH] Fix subproc listening when setting filter to non-t
 (Bug#36591)

* src/process.c (Fset_process_filter): Call add_process_read_fd
according to the state of process filter before it's updated.  This
restores the correct functioning as it was before 2016-02-16 "Allow
setting the filter masks later".  Inline the set_process_filter_masks
call instead of fixing it that function, because it is also called
from connect_network_socket, and we don't want to change the behavior
of that function so close to release.
---
 src/process.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/process.c b/src/process.c
index 2df51cfd99..b602507765 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1268,10 +1268,19 @@ DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
   if (NILP (filter))
     filter = Qinternal_default_process_filter;
 
-  pset_filter (p, filter);
-
   if (p->infd >= 0)
-    set_process_filter_masks (p);
+    {
+      /* If filter WILL be t, stop reading output.  */
+      if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
+        delete_read_fd (p->infd);
+      else if (/* If filter WAS t, then resume reading output.  */
+               EQ (p->filter, Qt)
+               /* Network or serial process not stopped:  */
+               && !EQ (p->command, Qt))
+        add_process_read_fd (p->infd);
+    }
+
+  pset_filter (p, filter);
 
   if (NETCONN1_P (p) || SERIALCONN1_P (p) || PIPECONN1_P (p))
     pset_childp (p, Fplist_put (p->childp, QCfilter, filter));
-- 
2.11.0


[-- Attachment #3: Type: text/plain, Size: 640 bytes --]


> On the master branch we should clean up the confusing set of if
> clauses, both in set-process-filter and in connect_network_socket.
> Perhaps Lars could describe his reasoning for making the change which
> introduced set_process_filter_masks and what problem it tried to
> solve.  (Btw, the log message for that change seems to imply that
> set-process-filter should not have called set_process_filter_masks,
> something that the change itself disagrees with.  An omission?)

Hmm, true, I didn't pay that close attention to the log message.
Maybe "we may not have a socket yet" refers to the already existing
'if (p->infd >= 0)' check?

bug#36591: 26.2; Term's pager seems broken

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>> On the master branch we should clean up the confusing set of if
>> clauses, both in set-process-filter and in connect_network_socket.
>> Perhaps Lars could describe his reasoning for making the change which
>> introduced set_process_filter_masks and what problem it tried to
>> solve.  (Btw, the log message for that change seems to imply that
>> set-process-filter should not have called set_process_filter_masks,
>> something that the change itself disagrees with.  An omission?)
>
> Hmm, true, I didn't pay that close attention to the log message.
> Maybe "we may not have a socket yet" refers to the already existing
> 'if (p->infd >= 0)' check?

Let's see...  this was part of the patch series that allowed for
asynchronous connection setup?

I think Noam is right -- the "we may not have the socket yet" refers to
this bit:

  if (p->infd >= 0)
    set_process_filter_masks (p);

But it does indeed look like I was confused with filter/p->filter and
assumed they were the same.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36591: 26.2; Term's pager seems broken

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  abliss@gmail.com,  36591@debbugs.gnu.org
> Date: Thu, 25 Jul 2019 12:02:09 +0200
> 
> Noam Postavsky <npostavs@gmail.com> writes:
> 
> >> On the master branch we should clean up the confusing set of if
> >> clauses, both in set-process-filter and in connect_network_socket.
> >> Perhaps Lars could describe his reasoning for making the change which
> >> introduced set_process_filter_masks and what problem it tried to
> >> solve.  (Btw, the log message for that change seems to imply that
> >> set-process-filter should not have called set_process_filter_masks,
> >> something that the change itself disagrees with.  An omission?)
> >
> > Hmm, true, I didn't pay that close attention to the log message.
> > Maybe "we may not have a socket yet" refers to the already existing
> > 'if (p->infd >= 0)' check?
> 
> Let's see...  this was part of the patch series that allowed for
> asynchronous connection setup?
> 
> I think Noam is right -- the "we may not have the socket yet" refers to
> this bit:
> 
>   if (p->infd >= 0)
>     set_process_filter_masks (p);

So you are saying that the commit log message wanted to explain the
code which existed there already?  Because the condition that tested
p->infd was already there before you refactored the code into
set_process_filter_masks.

That's somewhat strange, but I guess is OK.  However, I still wonder
what was the rationale for making the code change in the first place.
It seems to me that the real reason was the addition of the call to
set_process_filter_masks in connect_network_socket, but why was that
necessary?  IOW, what was missing from this code in
connect_network_socket, which already existed and manipulated some of
the same flags and descriptor bits:

  if (p->is_non_blocking_client)
    {
      /* We may get here if connect did succeed immediately.  However,
	 in that case, we still need to signal this like a non-blocking
	 connection.  */
      if (! (connecting_status (p->status)
	     && EQ (XCDR (p->status), addrinfos)))
	pset_status (p, Fcons (Qconnect, addrinfos));
      if ((fd_callback_info[inch].flags & NON_BLOCKING_CONNECT_FD) == 0)
	add_non_blocking_write_fd (inch);
    }
  else
    /* A server may have a client filter setting of Qt, but it must
       still listen for incoming connects unless it is stopped.  */
    if ((!EQ (p->filter, Qt) && !EQ (p->command, Qt))
	|| (EQ (p->status, Qlisten) && NILP (p->command)))
      add_process_read_fd (inch);

And in what use case did you see the need for the additional handling
in set_process_filter_masks?  I'd like to have a good understanding of
this, so that we could make this code less confusing on the master
branch.

TIA

bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> the "we may not have the socket yet" refers to this bit:
>
>   if (p->infd >= 0)
>     set_process_filter_masks (p);
>
> But it does indeed look like I was confused with filter/p->filter and
> assumed they were the same.

Do you think the reversed check in set_process_filter_masks could cause
bugs for sockets too then?

bug#36591: 26.2; Term's pager seems broken

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  abliss@gmail.com,  36591@debbugs.gnu.org
> Date: Wed, 24 Jul 2019 20:55:38 -0400
> 
> > I would like to leave connect_network_socket alone on the release
> > branch, and just fix the problem with set-process-filter.  I think the
> > easiest way is simply not to call set_process_filter_masks in
> > set-process-filter, and instead restore the way that code worked
> > before 9755b753, i.e. let it test the argument FILTER _before_ that
> > filter is installed.  Do you see any problems with this fix?
> 
> I think that makes sense, patch attached.

Thanks, this patch is OK for the emacs-26 branch.

> > On the master branch we should clean up the confusing set of if
> > clauses, both in set-process-filter and in connect_network_socket.
> > Perhaps Lars could describe his reasoning for making the change which
> > introduced set_process_filter_masks and what problem it tried to
> > solve.  (Btw, the log message for that change seems to imply that
> > set-process-filter should not have called set_process_filter_masks,
> > something that the change itself disagrees with.  An omission?)
> 
> Hmm, true, I didn't pay that close attention to the log message.
> Maybe "we may not have a socket yet" refers to the already existing
> 'if (p->infd >= 0)' check?

Let's see what Lars remembers, and let's then take it from there.

bug#36591: 26.2; Term's pager seems broken

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I think Noam is right -- the "we may not have the socket yet" refers to
>> this bit:
>> 
>>   if (p->infd >= 0)
>>     set_process_filter_masks (p);
>
> So you are saying that the commit log message wanted to explain the
> code which existed there already?  Because the condition that tested
> p->infd was already there before you refactored the code into
> set_process_filter_masks.
>
> That's somewhat strange, but I guess is OK.  However, I still wonder
> what was the rationale for making the code change in the first place.
> It seems to me that the real reason was the addition of the call to
> set_process_filter_masks in connect_network_socket, but why was that
> necessary?

Yes, it was refactored out into its own function so that we can call it
from connect_network_socket, too.

I think the logic is slowly coming back to me...  Lisp programs
typically call `set-process-filter' after calling
`make-network-process' -- even when opening an asynchronous connection.
This worked before because the connection wouldn't really be all that
synchronous -- it would do name resolution, and then open the socket, so
when `make-network-process' had returned, then p->infd would (almost)
always be valid.

When `make-network-process' was made more asynchronous, then p->infd
typically not be initialised yet, so `set-process-filter' would not do
the main bit any more.  To avoid having to rewrite all the callers of
`set-process-filter' with async connections, I just made it do the
filter setup in connect_network_socket, by which time we really have a
p->infd.

Logically speaking, it would make more sense if nobody called
`set-process-filter' until we have a connection, but that wouldn't be
backwards compatible.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36591: 26.2; Term's pager seems broken

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> the "we may not have the socket yet" refers to this bit:
>>
>>   if (p->infd >= 0)
>>     set_process_filter_masks (p);
>>
>> But it does indeed look like I was confused with filter/p->filter and
>> assumed they were the same.
>
> Do you think the reversed check in set_process_filter_masks could cause
> bugs for sockets too then?

Do you mean Unix domain socket?  Hm...  I don't think I follow...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Do you think the reversed check in set_process_filter_masks could cause
>> bugs for sockets too then?
>
> Do you mean Unix domain socket?  Hm...  I don't think I follow...

Not specifically, it's just that set_process_filter_masks is called in
two places, connect_network_socket and Fset_process_filter.  We're only
fixing the latter (in the release branch at least), so I was wondering
whether there could be problems stemming from the connect_network_socket
call.

bug#36591: 26.2; Term's pager seems broken

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>>> Do you think the reversed check in set_process_filter_masks could cause
>>> bugs for sockets too then?
>>
>> Do you mean Unix domain socket?  Hm...  I don't think I follow...
>
> Not specifically, it's just that set_process_filter_masks is called in
> two places, connect_network_socket and Fset_process_filter.  We're only
> fixing the latter (in the release branch at least), so I was wondering
> whether there could be problems stemming from the connect_network_socket
> call.

Oh, right.

Hm...  the problem was really when you call `set-process-filter' more
than once?  I do not think connect_network_socket is called more than
once per process...  so I don't think it should be a problem?  But the
logic is rather difficult to follow.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  the problem was really when you call `set-process-filter' more
> than once?

The problem is when you call (set-process-filter PROC t) and then
(set-process-filter PROC FILTER), where FILTER is not t.  The first time
we correctly do delete_read_fd, but on the second time, we don't call
add_process_read_fd on PROC's fd, so we never hear from it again.

For a minimal example with subprocesses (not sockets), see
https://debbugs.gnu.org/cgi/bugreport.cgi?msg=17;att=1;bug=36591;filename=bug-36591-proc-filter-t.el

> I do not think connect_network_socket is called more than once per
> process...  so I don't think it should be a problem?  But the logic is
> rather difficult to follow.

Yeah, quite difficult.  I guess the question is whether
connect_network_socket will call add_process_read_fd even the first time
though.  I think not.  But there are other calls to add_process_read_fd,
so it's possible that we end up listening to the socket anyway.

bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky]

fixed 36591 26.3
quit

Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> I think that makes sense, patch attached.
>
> Thanks, this patch is OK for the emacs-26 branch.

Okay, pushed.

b3e20737d8 2019-07-25T18:36:03-04:00 "Fix subproc listening when setting filter to non-t (Bug#36591)"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b3e20737d83acbbbec372645e2a951293d84bd29

bug#36591: 26.2; Term's pager seems broken

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> fixed 36591 26.3
> quit

I guess the intention was to close this bug, so I'm doing that now.
(I've only lightly skimmed the thread, but it seems like this commit
fixed the problem being discussed.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no