Subject: bug#78773: [PATCH] Speedup url-retrieve-synchronously for low-latency connections

From: Steven Allen <[email-masked]>
Date: Sat, 14 Jun 2025 17:38:34 -0700

Eli Zaretskii <[email-masked]> writes:

>> From: Steven Allen <[email-masked]>
>> Cc: [email-masked], [email-masked], [email-masked]
>> Date: Sat, 14 Jun 2025 12:37:38 -0700
>>
>> Eli Zaretskii <[email-masked]> writes:
>>
>> >> To be clear, calling `accept-process-output' with a nil process is the
>> >> exception, not the norm (8 out of 105 call sites).
>> >
>> > That might be, but in this particular case we changed the call to give
>> > it the nil argument for a reason, and it will be hard to convince me
>> > to go back on that decision.
>>
>> It was changed in 93e1248c2085dfb675d7ed916ec5621e3fe6e2c6 as a blind
>> attempt to fix a hard to reproduce bug. The commit message was:
>>
>>     * lisp/url/url.el (url-retrieve-synchronously): Use
>>     `accept-process-output' on a null process.  That is the safer, more
>>     conventional way of achieving non-blocking sleep-for (bug#49897).
>>
>> I understand if you want to leave well enough alone, but I also want to
>> find SOME way to fix this bug. Which leads us to...
>>
>> > So I suggest to explore other ways of solving this first.
>>
>> This is what I've been trying to do. There are only two ways to fix
>> this:
>>
>> 1. Change `accept-process-output' to not unconditionally wait an
>> additional 10ms after receiving output when PROCESS is nil "just in
>> case" there's some additional data to read.
>>
>> 2. Change `url-retrieve-synchronously' pass a non-nil PROCESS to
>> `accept-process-output' thereby avoiding the additional 10ms wait.
>>
>> I've been trying to go down the first path (I think that *is* the
>> correct path) but fixing it will necessarily require
>> `accept-process-output` to return immediately after receiving output
>> when PROCESS is nil, instead of waiting around in case some additional
>> output arrives.
>
> Taking a step back, you never explained how come waiting for 10 msec
> causes some operation to take a full second.  Could you please
> describe what happens in more details, so that such a significant
> effect of such a small delay could be understood without knowing what
> emacs-syncthing is or does?

emacs-syncthing makes 36+ RPC calls to the local syncthing process. Each
request takes ~1-2ms on my machine so the 10ms extra delay dominates the
request time.

However, you do bring up a good point as this change (either the first
or second patch) improves performance more than I'd expect:

1. Before this patch, opening the syncthing buffer took 900ms (much
greater than the expected 36*10ms).

2. After this patch, it takes 50ms (expected given 1-2ms for 36 RPC
calls plus the time to draw the buffer).

So it would appear that there's something going on here beyond the 10ms
delay from READ_OUTPUT_DELAY_INCREMENT (more like a 25ms delay). I'll do
some more digging.

> One aspect of this which I'd like to understand is how general is this
> problem and what kind of use patterns will bump into it.  Because if
> it turns out that just emacs-syncthing is affected, a simple solution
> would be for emacs-syncthing to have its own variant of
> url-retrieve-synchronously, in which case it can do anything it likes
> with its variant of that function.  By contrast, if we want to modify
> the code in Emacs, we need at least to understand what use cases need
> that and consider their importance vs the other kinds, which might
> suffer degradation in performance due to such a change.

This applies to any case where one makes multiple sequential RPC
requests to a local server, waiting on that request via
`accept-process-output' with a nil PROCESS.

However, there appears to be an escape hatch that lets, e.g., jsonrpc
avoid this issue: you can throw an error out of
`accept-process-output'. I've attached a patch that does this and it
appears to work, but throwing from a sentinel seems sketchy.

>From 2a7c99cef6644984bc7c240f925171ba14dc089c Mon Sep 17 00:00:00 2001
From: Steven Allen <[email-masked]>
Date: Sat, 14 Jun 2025 15:27:22 -0700
Subject: [PATCH] Rewrite url-retrieve-synchronously to throw when done

When called with no PROCESS, `accept-process-output' waits ~10ms after
first receiving process output before returning, adding (at least) 10ms
of latency to any `url-retrieve-synchronously' calls.

This patch avoids this by throwing from the network process sentinel and
catching it in the outer `url-retrieve-synchronously', thereby returning
immediately when the response is ready.
---
 lisp/url/url.el | 93 ++++++++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 48 deletions(-)

diff --git a/lisp/url/url.el b/lisp/url/url.el
index 090a952cf4c..995c97a1a2f 100644
--- a/lisp/url/url.el
+++ b/lisp/url/url.el
@@ -235,58 +235,55 @@ url-retrieve-synchronously
 TIMEOUT is passed, it should be a number that says (in seconds)
 how long to wait for a response before giving up."
   (url-do-setup)
-  (let* (;; Ensure we can stop during connection setup (bug#71295).
+  (let* ((tag (gensym "url-retrieve-synchronously-tag"))
+         ;; Ensure we can stop during connection setup (bug#71295).
          (url-asynchronous (not (null timeout)))
-         data-buffer
-         (callback (lambda (&rest _args)
-                     (setq data-buffer (current-buffer))
-                     (url-debug 'retrieval
-                                "Synchronous fetching done (%S)"
-                                data-buffer)))
+         (throw-on-input nil)
          (start-time (current-time))
-         (proc-buffer (url-retrieve url callback nil silent
-                                    inhibit-cookies)))
-    (if (not proc-buffer)
+         (callback (lambda (&rest _args)
+                     (let ((data-buffer (current-buffer)))
+                       (url-debug 'retrieval
+                                  "Synchronous fetching done (%S)"
+                                  data-buffer)
+                       (throw tag data-buffer))))
+         proc-buffer)
+    (catch tag
+      (setq proc-buffer (url-retrieve url callback nil silent
+                                         inhibit-cookies))
+      (unless proc-buffer
         (url-debug 'retrieval "Synchronous fetching unnecessary %s" url)
+        (throw nil))
+
       (unwind-protect
-          (catch 'done
-            (while (not data-buffer)
-              (when (and timeout (time-less-p timeout
-                                              (time-since start-time)))
-                (url-debug 'retrieval "Timed out %s (after %ss)" url
-                           (float-time (time-since start-time)))
-                (throw 'done 'timeout))
-	      (url-debug 'retrieval
-		         "Spinning in url-retrieve-synchronously: nil (%S)"
-		         proc-buffer)
-              (when-let* ((redirect-buffer
-                           (buffer-local-value 'url-redirect-buffer
-                                               proc-buffer)))
-                (unless (eq redirect-buffer proc-buffer)
-                  (url-debug
-                   'retrieval "Redirect in url-retrieve-synchronously: %S -> %S"
-		   proc-buffer redirect-buffer)
-                  (let (kill-buffer-query-functions)
-                    (kill-buffer proc-buffer))
-                  ;; Accommodate hack in commit 55d1d8b.
-                  (setq proc-buffer redirect-buffer)))
-              (when-let* ((proc (get-buffer-process proc-buffer)))
-                (when (memq (process-status proc)
-                            '(closed exit signal failed))
-                  ;; Process sentinel vagaries occasionally cause
-                  ;; url-retrieve to fail calling callback.
-                  (unless data-buffer
-                    (url-debug 'retrieval "Dead process %s" url)
-		    (throw 'done 'exception))))
-              ;; Querying over consumer internet in the US takes 100
-              ;; ms, so split the difference.
-              (accept-process-output nil 0.05)))
-        ;; Kill the process buffer on redirects.
-        (when (and data-buffer
-                   (not (eq data-buffer proc-buffer)))
-          (let (kill-buffer-query-functions)
-            (kill-buffer proc-buffer)))))
-    data-buffer))
+          (while t
+            (when (and timeout (time-less-p timeout (time-since start-time)))
+              (url-debug 'retrieval "Timed out %s (after %ss)" url
+                         (float-time (time-since start-time)))
+              (throw tag 'timeout))
+            (url-debug 'retrieval
+                       "Spinning in url-retrieve-synchronously: nil (%S)"
+                       proc-buffer)
+            ;; Querying over consumer internet in the US takes 100
+            ;; ms, so split the difference.
+            (accept-process-output nil 0.05))
+        ;; Cleanup
+        (let ((buf proc-buffer))
+          (while buf
+            ;; If there are still running processes, we must have
+            ;; canceled the request (quit) and/or timed out. Kill the
+            ;; process.
+            (when-let* ((proc (get-buffer-process buf)))
+              ;; Catch & ignore throws from the sentinel (can happen if
+              ;; the process is still live).
+              (catch tag (delete-process proc)))
+            ;; Follow the chain of redirect buffers and clean them up
+            ;; one by one.
+            (setq buf (when-let* ((redirect-buffer
+                                   (buffer-local-value
+                                    'url-redirect-buffer buf)))
+                        (let (kill-buffer-query-functions)
+                          (kill-buffer proc-buffer))
+                        redirect-buffer))))))))
 
 ;; url-mm-callback called from url-mm, which requires mm-decode.
 (declare-function mm-dissect-buffer "mm-decode"
-- 
2.49.0