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)