Re: bug#62044: 30.0.50; ERC 5.5: Auto-reconnect is broken

--=-=-=
Content-Type: text/plain

"J.P." <[email protected]> writes:

> At the end of the day, I still question the soundness of resorting to a
> protocol exchange to prove that the underlying transport is functioning
> properly. However, if no one complains in the next week or two, I'll
> close this with the release of 5.6.1.

This "delayed check" method has reportedly been failing for some users.
It apparently "hangs" at some point while redialing and displays a
negative countdown in the mode line segment. If my suspicions are
correct, this is somewhat of a known issue related to the sending of a
"stateless" pre-connection-registration message, namely a PING, which is
pretty nonstandard for clients.

One possible solution that springs to mind involves reusing the last
session's local state to perform the initial "$hello" of the so-called
"connection registration" sequence. It would interpret any response as
indicative of a healthy connection and stash it in order to first
initialize the session properly before processing the stashed response
manually at the juncture where its request (the "$hello" thing) would
normally be emitted. This solution is appealing in that it would
preserve the program's basic design and contracts. However, its fatal
flaw would be performing connection registration in the old session's
buffer. Doing this hampers modularity by making it difficult if not
impossible for modules to meaningfully influence the process. In any
case, the second of the attached patches (the one labeled "POC") demos
this approach.

For now, I think our best option is to forgo emitting _any_ pre-
registration protocol and to instead accept the "open\n" event from the
process sentinel as sufficient evidence our messages are getting
through. Of course, not seeing a single response means this reconnect
method may still fail with some proxies, which is something I guess
we'll just have to live with. Anyway, the first of the attached patches
(the one labeled "5.6.1") hopefully does this in a competent way. Those
affected by this bug are encouraged to give it a try.

In the end, the fact remains that anything short of fully integrating
the reconnect logic into session setup and the connection-registration
dialog will always be a hack and prone to failure. Unfortunately, such a
comprehensive overhaul would mean reworking the way ERC initializes
sessions and would break way too much user code. But it's something
perhaps worth revisiting on a major version release.

Thanks.


--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment;
 filename=0001-5.6.1-Don-t-round-trip-auto-reconnect-probe-in-ERC.patch

From c52b7e0121a5146bc68cd35d8fa55d2691dc1af8 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email protected]>
Date: Tue, 8 Apr 2025 23:17:21 -0700
Subject: [PATCH] [5.6.1] Don't round trip auto-reconnect probe in ERC

* lisp/erc/erc-backend.el (erc--recon-probe-sentinel): Run
`erc-server--reconnect-opened' immediately because sending a speculative
PING doesn't work on all servers and proxies, most crucially on ZNC,
which replies with an error only after an extended timeout.
(erc--recon-probe-filter): Remove unused function.
(erc-server-delayed-check-reconnect): Set filter function to `ignore'.
(Bug#62044)
---
 lisp/erc/erc-backend.el | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index e9b39a6f3f4..dd3532769d3 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -869,11 +869,10 @@ erc--recon-probe-sentinel
 Otherwise, try connecting from scratch again after timeout."
   (pcase event
     ("open\n"
-     (let ((cookie (time-convert nil 'integer)))
-       (process-put proc 'erc--reconnect-cookie cookie)
-       ;; FIXME account for possible `file-error' when sending.
-       (run-at-time nil nil #'process-send-string proc
-                    (format "PING %d\r\n" cookie))))
+     (set-process-sentinel proc #'ignore)
+     ;; FIXME account for possible `file-error' when sending.
+     (run-at-time nil nil #'erc-server--reconnect-opened
+                  (process-buffer proc) proc))
     ((and "connection broken by remote peer\n"
           (guard (process-get proc 'erc--reconnect-cookie))
           (let buffer (process-buffer proc))
@@ -894,27 +893,10 @@ erc--recon-probe-sentinel
     ((or "connection broken by remote peer\n" (rx bot "failed"))
      (run-at-time nil nil #'erc--recon-probe-reschedule proc))))
 
-(defun erc--recon-probe-filter (proc string)
-  "Reconnect, reusing PROC if STRING contains a \"PONG\"."
-  (when-let* ((buffer (process-buffer proc))
-              (buffer-live-p buffer))
-    (with-current-buffer buffer
-      (setq erc--server-reconnect-timeout nil))
-    (if-let* ; reuse proc if string has complete message
-        ((cookie (process-get proc 'erc--reconnect-cookie))
-         ;; Accommodate a leading ":<source> ".
-         ((string-suffix-p (format "PONG %d\r\n" cookie) string)))
-        (progn
-          (erc-log-irc-protocol string nil)
-          (set-process-sentinel proc #'ignore)
-          (set-process-filter proc nil)
-          (run-at-time nil nil #'erc-server--reconnect-opened buffer proc))
-      (delete-process proc)
-      (run-at-time nil nil #'erc-server-delayed-reconnect buffer))))
-
 (defun erc--recon-probe-check (proc tmrx)
   "Restart auto-reconnect probe if PROC has failed or TIMER has EXPIRE'd.
 Expect TMRX to be a cons cell of (EXPIRE . TIMER)."
+  (cl-assert (cdr tmrx))
   (let* ((status (process-status proc))
          (expiredp (time-less-p (pop tmrx) (current-time)))
          (buffer (process-buffer proc)))
@@ -961,7 +943,7 @@ erc-server-delayed-check-reconnect
                               nil server erc-session-port
                               (and cert (list :client-certificate cert)))))
             (setcdr tmrx (run-at-time 1 1 #'erc--recon-probe-check proc tmrx))
-            (set-process-filter proc #'erc--recon-probe-filter)
+            (set-process-filter proc #'ignore)
             (set-process-sentinel proc #'erc--recon-probe-sentinel)
             (set-process-buffer proc buffer)
             ;; Should `erc-server-process' also be set to `proc' here so
-- 
2.49.0


--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment;
 filename=0001-POC-Send-full-erc-login-as-reconnect-probe.patch

From a1fb4ac024281131be79037a0968ee7317c84ceb Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email protected]>
Date: Tue, 8 Apr 2025 23:27:33 -0700
Subject: [PATCH] [POC] Send full erc-login as reconnect probe

---
 lisp/erc/erc-backend.el | 72 +++++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 21 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index e9b39a6f3f4..2273fc56ab1 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -713,9 +713,22 @@ erc-open-network-stream
     (apply #'open-network-stream name buffer host service p)))
 
 (cl-defmethod erc--register-connection ()
-  "Perform opening IRC protocol exchange with server."
+  "Perform opening IRC protocol exchange with server.
+If `erc-server-filter-data' has been populated, run the server filter
+because `erc-login' must have already run.  Otherwise, run `erc-login'."
   (run-hooks 'erc--server-post-connect-hook)
-  (erc-login))
+  (if erc-server-filter-data
+      (run-at-time nil nil #'erc-server-filter-function erc-server-process "")
+    (erc-login)))
+
+(defvar erc--reconnect-registration-out nil
+  "Outgoing lines from speculative `erc-login' used as dial probe.")
+
+(cl-defmethod erc--server-send
+  (string _force _target &context (erc--reconnect-registration-out cons))
+  "Defer sending STRING by adding to `erc--reconnect-registration-out'."
+  (erc-log-irc-protocol string 'outbound)
+  (push string erc--reconnect-registration-out))
 
 (defvar erc--server-post-dial-function
   #'erc--server-propagate-failed-connection
@@ -764,7 +777,9 @@ erc-server-connect
       (error "Connection attempt failed"))
     ;; Misc server variables
     (with-current-buffer buffer
-      (setq erc-server-filter-data nil)
+      (setq erc-server-filter-data
+            (prog1 (process-get process 'erc--reconnect-registration-in)
+              (process-put process 'erc--reconnect-registration-in nil)))
       (setq erc-server-process process)
       (setq erc-server-quitting nil)
       (setq erc-server-reconnecting nil
@@ -864,16 +879,29 @@ erc--recon-probe-reschedule
                                'recon-probe-nobody-home)
           (erc-schedule-reconnect buffer 0))))))
 
+(defun erc--recon-probe-pipeline-registration (proc)
+  "Send concatenated lines emitted by `erc--register-connection' to PROC."
+  (when-let* ((buffer (process-buffer proc))
+              ((buffer-live-p buffer)))
+    (with-current-buffer buffer
+      (let ((erc--reconnect-registration-out (list t)))
+        (let ((inhibit-message t)
+              (erc--server-post-connect-hook nil))
+          (erc--register-connection))
+        (cl-assert erc--reconnect-registration-out)
+        (process-send-string
+         proc
+         (string-join
+          (cdr (nreverse (cons "" erc--reconnect-registration-out)))
+          "\r\n"))))))
+
 (defun erc--recon-probe-sentinel (proc event)
   "Send a \"PING\" to PROC's peer on an \"open\" EVENT.
 Otherwise, try connecting from scratch again after timeout."
   (pcase event
     ("open\n"
-     (let ((cookie (time-convert nil 'integer)))
-       (process-put proc 'erc--reconnect-cookie cookie)
-       ;; FIXME account for possible `file-error' when sending.
-       (run-at-time nil nil #'process-send-string proc
-                    (format "PING %d\r\n" cookie))))
+     ;; FIXME account for possible `file-error' when sending.
+     (run-at-time nil nil #'erc--recon-probe-pipeline-registration proc))
     ((and "connection broken by remote peer\n"
           (guard (process-get proc 'erc--reconnect-cookie))
           (let buffer (process-buffer proc))
@@ -895,22 +923,24 @@ erc--recon-probe-sentinel
      (run-at-time nil nil #'erc--recon-probe-reschedule proc))))
 
 (defun erc--recon-probe-filter (proc string)
-  "Reconnect, reusing PROC if STRING contains a \"PONG\"."
+  "Reconnect, reusing PROC.  Stash STRING and maybe reconnect.
+Append STRING to the `erc--reconnect-registration-in' symbol property of
+PROC."
   (when-let* ((buffer (process-buffer proc))
               (buffer-live-p buffer))
     (with-current-buffer buffer
-      (setq erc--server-reconnect-timeout nil))
-    (if-let* ; reuse proc if string has complete message
-        ((cookie (process-get proc 'erc--reconnect-cookie))
-         ;; Accommodate a leading ":<source> ".
-         ((string-suffix-p (format "PONG %d\r\n" cookie) string)))
-        (progn
-          (erc-log-irc-protocol string nil)
-          (set-process-sentinel proc #'ignore)
-          (set-process-filter proc nil)
-          (run-at-time nil nil #'erc-server--reconnect-opened buffer proc))
-      (delete-process proc)
-      (run-at-time nil nil #'erc-server-delayed-reconnect buffer))))
+      (setq erc--server-reconnect-timeout nil)
+      ;; Log a full message.
+      (when (string-suffix-p "\r\n" string)
+        (erc-log-irc-protocol string nil))
+      ;; Reconnection succeeded, so stop checking.
+      (set-process-sentinel proc #'ignore)
+      (let ((existing (process-get proc 'erc--reconnect-registration-in)))
+        (process-put proc 'erc--reconnect-registration-in
+                     (concat existing string))
+        (unless existing
+          (run-at-time nil nil #'erc-server--reconnect-opened
+                       buffer proc))))))
 
 (defun erc--recon-probe-check (proc tmrx)
   "Restart auto-reconnect probe if PROC has failed or TIMER has EXPIRE'd.
-- 
2.49.0


--=-=-=--