* bug#36591: 26.2; Term's pager seems broken @ 2019-07-11 0:06 Adam Bliss 2019-07-23 13:53 ` Noam Postavsky 0 siblings, 1 reply; 18+ messages in thread From: Adam Bliss @ 2019-07-11 0:06 UTC (permalink / raw) To: 36591 [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-11 0:06 bug#36591: 26.2; Term's pager seems broken Adam Bliss @ 2019-07-23 13:53 ` Noam Postavsky 2019-07-23 17:40 ` Eli Zaretskii 0 siblings, 1 reply; 18+ messages in thread From: Noam Postavsky @ 2019-07-23 13:53 UTC (permalink / raw) To: Adam Bliss; +Cc: 36591 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); ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-23 13:53 ` Noam Postavsky @ 2019-07-23 17:40 ` Eli Zaretskii 2019-07-23 22:33 ` Adam Bliss 2019-07-24 2:07 ` Noam Postavsky 0 siblings, 2 replies; 18+ messages in thread From: Eli Zaretskii @ 2019-07-23 17:40 UTC (permalink / raw) To: Noam Postavsky; +Cc: abliss, 36591 > 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-23 17:40 ` Eli Zaretskii @ 2019-07-23 22:33 ` Adam Bliss 2019-07-24 2:07 ` Noam Postavsky 1 sibling, 0 replies; 18+ messages in thread From: Adam Bliss @ 2019-07-23 22:33 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Noam Postavsky, 36591 [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-23 17:40 ` Eli Zaretskii 2019-07-23 22:33 ` Adam Bliss @ 2019-07-24 2:07 ` Noam Postavsky 2019-07-24 15:07 ` Eli Zaretskii 1 sibling, 1 reply; 18+ messages in thread From: Noam Postavsky @ 2019-07-24 2:07 UTC (permalink / raw) To: Eli Zaretskii; +Cc: abliss, 36591 [-- 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"). ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-24 2:07 ` Noam Postavsky @ 2019-07-24 15:07 ` Eli Zaretskii 2019-07-25 0:55 ` Noam Postavsky 0 siblings, 1 reply; 18+ messages in thread From: Eli Zaretskii @ 2019-07-24 15:07 UTC (permalink / raw) To: Noam Postavsky, Lars Ingebrigtsen; +Cc: abliss, 36591 > 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?) ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-24 15:07 ` Eli Zaretskii @ 2019-07-25 0:55 ` Noam Postavsky 2019-07-25 10:02 ` Lars Ingebrigtsen 2019-07-25 13:11 ` Eli Zaretskii 0 siblings, 2 replies; 18+ messages in thread From: Noam Postavsky @ 2019-07-25 0:55 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Lars Ingebrigtsen, abliss, 36591 [-- 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? ^ permalink raw reply related [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 0:55 ` Noam Postavsky @ 2019-07-25 10:02 ` Lars Ingebrigtsen 2019-07-25 13:01 ` Eli Zaretskii 2019-07-25 13:01 ` Noam Postavsky 2019-07-25 13:11 ` Eli Zaretskii 1 sibling, 2 replies; 18+ messages in thread From: Lars Ingebrigtsen @ 2019-07-25 10:02 UTC (permalink / raw) To: Noam Postavsky; +Cc: abliss, 36591 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 10:02 ` Lars Ingebrigtsen @ 2019-07-25 13:01 ` Eli Zaretskii 2019-07-25 16:58 ` Lars Ingebrigtsen 2019-07-25 13:01 ` Noam Postavsky 1 sibling, 1 reply; 18+ messages in thread From: Eli Zaretskii @ 2019-07-25 13:01 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: abliss, 36591, npostavs > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 13:01 ` Eli Zaretskii @ 2019-07-25 16:58 ` Lars Ingebrigtsen 0 siblings, 0 replies; 18+ messages in thread From: Lars Ingebrigtsen @ 2019-07-25 16:58 UTC (permalink / raw) To: Eli Zaretskii; +Cc: abliss, 36591, npostavs 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 10:02 ` Lars Ingebrigtsen 2019-07-25 13:01 ` Eli Zaretskii @ 2019-07-25 13:01 ` Noam Postavsky 2019-07-25 17:01 ` Lars Ingebrigtsen 1 sibling, 1 reply; 18+ messages in thread From: Noam Postavsky @ 2019-07-25 13:01 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: abliss, 36591 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 13:01 ` Noam Postavsky @ 2019-07-25 17:01 ` Lars Ingebrigtsen 2019-07-25 17:28 ` Noam Postavsky 0 siblings, 1 reply; 18+ messages in thread From: Lars Ingebrigtsen @ 2019-07-25 17:01 UTC (permalink / raw) To: Noam Postavsky; +Cc: abliss, 36591 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 17:01 ` Lars Ingebrigtsen @ 2019-07-25 17:28 ` Noam Postavsky 2019-07-25 17:44 ` Lars Ingebrigtsen 0 siblings, 1 reply; 18+ messages in thread From: Noam Postavsky @ 2019-07-25 17:28 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: abliss, 36591 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 17:28 ` Noam Postavsky @ 2019-07-25 17:44 ` Lars Ingebrigtsen 2019-07-25 17:57 ` Noam Postavsky 0 siblings, 1 reply; 18+ messages in thread From: Lars Ingebrigtsen @ 2019-07-25 17:44 UTC (permalink / raw) To: Noam Postavsky; +Cc: abliss, 36591 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 17:44 ` Lars Ingebrigtsen @ 2019-07-25 17:57 ` Noam Postavsky 0 siblings, 0 replies; 18+ messages in thread From: Noam Postavsky @ 2019-07-25 17:57 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: abliss, 36591 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 0:55 ` Noam Postavsky 2019-07-25 10:02 ` Lars Ingebrigtsen @ 2019-07-25 13:11 ` Eli Zaretskii 2019-07-25 22:38 ` Noam Postavsky 1 sibling, 1 reply; 18+ messages in thread From: Eli Zaretskii @ 2019-07-25 13:11 UTC (permalink / raw) To: Noam Postavsky; +Cc: larsi, abliss, 36591 > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 13:11 ` Eli Zaretskii @ 2019-07-25 22:38 ` Noam Postavsky 2020-08-21 12:58 ` Lars Ingebrigtsen 0 siblings, 1 reply; 18+ messages in thread From: Noam Postavsky @ 2019-07-25 22:38 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, abliss, 36591 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#36591: 26.2; Term's pager seems broken 2019-07-25 22:38 ` Noam Postavsky @ 2020-08-21 12:58 ` Lars Ingebrigtsen 0 siblings, 0 replies; 18+ messages in thread From: Lars Ingebrigtsen @ 2020-08-21 12:58 UTC (permalink / raw) To: Noam Postavsky; +Cc: abliss, 36591 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-08-21 12:58 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-11 0:06 bug#36591: 26.2; Term's pager seems broken Adam Bliss 2019-07-23 13:53 ` Noam Postavsky 2019-07-23 17:40 ` Eli Zaretskii 2019-07-23 22:33 ` Adam Bliss 2019-07-24 2:07 ` Noam Postavsky 2019-07-24 15:07 ` Eli Zaretskii 2019-07-25 0:55 ` Noam Postavsky 2019-07-25 10:02 ` Lars Ingebrigtsen 2019-07-25 13:01 ` Eli Zaretskii 2019-07-25 16:58 ` Lars Ingebrigtsen 2019-07-25 13:01 ` Noam Postavsky 2019-07-25 17:01 ` Lars Ingebrigtsen 2019-07-25 17:28 ` Noam Postavsky 2019-07-25 17:44 ` Lars Ingebrigtsen 2019-07-25 17:57 ` Noam Postavsky 2019-07-25 13:11 ` Eli Zaretskii 2019-07-25 22:38 ` Noam Postavsky 2020-08-21 12:58 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.