Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[Stefan Monnier]

>     Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

BTW, this has bitten us in various other cases, we should fix it once
and for all in the C code by marking the process as "busy" while we're
running the filters so we never run filters recursively.
[ If we ever bump into a case where recursive filters are needed, we
  can then add some function to remove the "busy" mark.
  Calling (accept-process-output PROC) should naturally mark PROC as
  not-busy.  ]


        Stefan

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[João Távora]

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

On Fri, Dec 16, 2022 at 2:36 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> >     Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)
>
> BTW, this has bitten us in various other cases, we should fix it once
> and for all in the C code by marking the process as "busy" while we're
> running the filters so we never run filters recursively.
> [ If we ever bump into a case where recursive filters are needed, we
>   can then add some function to remove the "busy" mark.
>   Calling (accept-process-output PROC) should naturally mark PROC as
>   not-busy.  ]


I thought about that too, but I don't know what other implications
it might have. If it's somehow feasible, isn't it easier to just change
process-send-input to _not_ accept output?  That's the
only output-accepting primitive that makes sense to call from
within a process filter (and that's what happened here).  I can't
see how sit-for is useful from within a process-filter and neither
accept-process-output.  Almost certainly a bug, right? So why
can't we make it so that they just error when called under those
circumstances?

João

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

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[Stefan Monnier]

> I thought about that too, but I don't know what other implications
> it might have. If it's somehow feasible, isn't it easier to just change
> process-send-input to _not_ accept output?  That's the
> only output-accepting primitive that makes sense to call from
> within a process filter (and that's what happened here).  I can't
> see how sit-for is useful from within a process-filter and neither
> accept-process-output.  Almost certainly a bug, right? So why
> can't we make it so that they just error when called under those
> circumstances?

To me, the only thing that *is* sure is that recursive invocations of
process filters are usually not expected (and because of
non-determinism they're usually not guaranteed either), so I'd focus
on this.

Trying to figure out when `process-send-input` should or shouldn't
accept process output (and from which processes) seems a lot more murky.

And I know `accept-process-output` can be (and is) used in
process filters.  I'd be surprised if the same didn't happen for
`sit-for`.


        Stefan

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[F. Jason Park]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)
>
> BTW, this has bitten us in various other cases, we should fix it once
> and for all in the C code by marking the process as "busy" while we're
> running the filters so we never run filters recursively.
> [ If we ever bump into a case where recursive filters are needed, we
>   can then add some function to remove the "busy" mark.
>   Calling (accept-process-output PROC) should naturally mark PROC as
>   not-busy.  ]
>
>
>         Stefan

A possibly related discussion from the bug archive:

  https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-03/msg01211.html

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[Stefan Monnier]

F. Jason Park [2022-12-16 21:37:36] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>     Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)
>>
>> BTW, this has bitten us in various other cases, we should fix it once
>> and for all in the C code by marking the process as "busy" while we're
>> running the filters so we never run filters recursively.
>> [ If we ever bump into a case where recursive filters are needed, we
>>   can then add some function to remove the "busy" mark.
>>   Calling (accept-process-output PROC) should naturally mark PROC as
>>   not-busy.  ]
>>
>>
>>         Stefan
>
> A possibly related discussion from the bug archive:
>
>   https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-03/msg01211.html

Indeed.  I think this points to the need to "spawn" a piece of code to
be executed "ASAP" but not necessarily immediately.

This way when a process filter needs to send something in response to
what it received, it can just "spawn" the send, so we can return from
the process filter before the send finishes.


        Stefan

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[João Távora]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> F. Jason Park [2022-12-16 21:37:36] wrote:
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>>     Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)
>>>
>>> BTW, this has bitten us in various other cases, we should fix it once
>>> and for all in the C code by marking the process as "busy" while we're
>>> running the filters so we never run filters recursively.
>>> [ If we ever bump into a case where recursive filters are needed, we
>>>   can then add some function to remove the "busy" mark.
>>>   Calling (accept-process-output PROC) should naturally mark PROC as
>>>   not-busy.  ]
>>>
>>>
>>>         Stefan
>>
>> A possibly related discussion from the bug archive:
>>
>>   https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-03/msg01211.html
>
> Indeed.  I think this points to the need to "spawn" a piece of code to
> be executed "ASAP" but not necessarily immediately.

Would that be sth like

  (if in-process-filter (run-at-time 0 nil #'piece-of-code) (piece-of-code))

? ... supposing in-process-filter existed, of course.

>
> This way when a process filter needs to send something in response to
> what it received, it can just "spawn" the send, so we can return from
> the process filter before the send finishes.

I guess you can see it that way too.  So there are two ways to solve
this:

* only process-send-input in process filters makes sense
* all but process-send-input in process filters makes sense

I'm more into of the first persuasion, but I think it shouldn't allow
output to be accepted when called from within a process filter.  But I
guess the second option isn't bad too.  I've rehearsed a patch to
jsonrpc.el that uses this idea instead of the other kill recursion one,
and maybe it's cleaner, indeed.  Would appreciate feedback.

diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
index 2d562610b3..e72644d317 100644
--- a/lisp/jsonrpc.el
+++ b/lisp/jsonrpc.el
@@ -418,6 +418,9 @@ initialize-instance
       (setq buffer-read-only t))
     (process-put proc 'jsonrpc-connection conn)))
 
+(defvar jsonrpc--in-process-filter nil
+  "Non-nil if inside `jsonrpc--process-filter'.")
+
 (cl-defmethod jsonrpc-connection-send ((connection jsonrpc-process-connection)
                                        &rest args
                                        &key
@@ -437,13 +440,16 @@ jsonrpc-connection-send
          (headers
           `(("Content-Length" . ,(format "%d" (string-bytes json)))
             ;; ("Content-Type" . "application/vscode-jsonrpc; charset=utf-8")
-             )))
+            ))
+         (send-it
+          (lambda ()
             (process-send-string
              (jsonrpc--process connection)
              (cl-loop for (header . value) in headers
                       concat (concat header ": " value "\r\n") into header-section
                       finally return (format "%s\r\n%s" header-section json)))
-    (jsonrpc--log-event connection message 'client)))
+            (jsonrpc--log-event connection message 'client))))
+    (if jsonrpc--in-process-filter (run-at-time 0 nil send-it) (funcall send-it))))
 
 (defun jsonrpc-process-type (conn)
   "Return the `process-type' of JSONRPC connection CONN."
@@ -548,22 +554,8 @@ jsonrpc--process-sentinel
         (delete-process proc)
         (funcall (jsonrpc--on-shutdown connection) connection)))))
 
-(defvar jsonrpc--in-process-filter nil
-  "Non-nil if inside `jsonrpc--process-filter'.")
-
 (cl-defun jsonrpc--process-filter (proc string)
   "Called when new data STRING has arrived for PROC."
-  (when jsonrpc--in-process-filter
-    ;; Problematic recursive process filters may happen if
-    ;; `jsonrpc--connection-receive', called by us, eventually calls
-    ;; client code which calls `process-send-string' (which see) to,
-    ;; say send a follow-up message.  If that happens to writes enough
-    ;; bytes for pending output to be received, we will lose JSONRPC
-    ;; messages.  In that case, remove recursiveness by re-scheduling
-    ;; ourselves to run from within a timer as soon as possible
-    ;; (bug#60088)
-    (run-at-time 0 nil #'jsonrpc--process-filter proc string)
-    (cl-return-from jsonrpc--process-filter))
   (when (buffer-live-p (process-buffer proc))
     (with-current-buffer (process-buffer proc)
       (let* ((inhibit-read-only t)

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[Stefan Monnier]

>> Indeed.  I think this points to the need to "spawn" a piece of code to
>> be executed "ASAP" but not necessarily immediately.
>
> Would that be sth like
>
>   (if in-process-filter (run-at-time 0 nil #'piece-of-code) (piece-of-code))
>
> ? ... supposing in-process-filter existed, of course.

I was thinking of something more like unconditonally

    (run-at-time 0 nil #'piece-of-code)

tho abstracted behind a function.

>> This way when a process filter needs to send something in response to
>> what it received, it can just "spawn" the send, so we can return from
>> the process filter before the send finishes.
>
> I guess you can see it that way too.  So there are two ways to solve
> this:
>
> * only process-send-input in process filters makes sense
> * all but process-send-input in process filters makes sense

I assume you meant to write `process-send-string`, but I don't know what
you mean by the above (I understand neither bullets).

> I'm more into of the first persuasion, but I think it shouldn't allow
> output to be accepted when called from within a process filter.

Indeed, as a general rule doing a "blocking wait", such as
`accept-process-output` from within async code (process filter, timer,
etc..) is generally undesirable.


        Stefan

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[João Távora]

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

On Sun, Dec 18, 2022 at 1:57 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> I guess you can see it that way too.  So there are two ways to solve
> > this:
> >
> > * only process-send-input in process filters makes sense
> > * all but process-send-input in process filters makes sense
>
> I assume you meant to write `process-send-string`, but I don't know what
> you mean by the above (I understand neither bullets).
>

Yes, it's process-send-string.  I talked earlier about how I think sit-for
and
accept-process-output are two primitives that could just error when
called from within a process filter, because there's no possible reasonable
use for them, because they lead to recursive filters and recursive filters
are arguably "unreasonable".  But process-send-string (without
output-acceptance) in a filter makes sense.  That's the first bullet.

The other bullet is that a-p-output and sit-for and even recursive filters
may
make sense (if you know what you are doing, i suppose), but the common
case of sending a follow up message to a process (as a direct consequence
of the output just received) should _not_ be handled with an in-filter
p-s-string,
but with something else, like the run-at-timer idea.  In that case we can
keep
process-send-string's output acceptance.


> > I'm more into of the first persuasion, but I think it shouldn't allow
> > output to be accepted when called from within a process filter.
>
> Indeed, as a general rule doing a "blocking wait", such as
> `accept-process-output` from within async code (process filter, timer,
> etc..) is generally undesirable.
>

I agree, but process-send-string is never blocking, is it?  And anyway
if we go your 'spawn' or 'run-asap' way, we don't need to change
process-send-string's output-acceptance semantics at all.

João

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

Re: emacs-29 8bf4cdcf79: Avoid recursive process filters in lisp/jsonrpc.el (bug#60088)

[Stefan Monnier]

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

> Yes, it's process-send-string.  I talked earlier about how I think
> sit-for and accept-process-output are two primitives that could just
> error when called from within a process filter, because there's no
> possible reasonable use for them, because they lead to recursive
> filters and recursive filters are arguably "unreasonable".

I agree with the sentiment, but it's not realistic given the amount of
existing code which does that, I think (and the amount of work to fix them).
We could have maybe warnings or somesuch to detect those places and start
fixing them, but I think we also need to up our game in terms of the
infrastucture we provide to help write "correct" async code (see
`futur.el` below which exposes my current ideas).

> But process-send-string (without output-acceptance) in a filter
> makes sense.

You mean "non-blocking"?  Yes, we need a non-blocking variant of `process-send-string`.

> I agree, but process-send-string is never blocking, is it?

It is, currently (which is why it can accept process output in the mean time).

> And anyway if we go your 'spawn' or 'run-asap' way, we don't need to
> change process-send-string's output-acceptance semantics at all.

We do, because when that timer triggers, your Emacs will be unresponsive
while `process-send-string` is running (which can take arbitrarily long
if the process is busy doing other things than reading our string).


        Stefan

[-- Attachment #2: futur.el --]
[-- Type: application/emacs-lisp, Size: 11105 bytes --]