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

["J.P." <jp@neverwas.me>]

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

"J.P." <jp@neverwas.me> writes:

> I've attached a less sloppy version that probably still fails in some
> common cases, but at least it reuses the existing session connector.

I've improved upon this further (v3 attached) by adding a housekeeping
task to monitor the initial server process from creation. Such a move
may be regrettable because it adds yet more complexity to the already
dizzying auto-reconnect landscape. However, I couldn't find a suitable
way to cover common process errors that aren't presented to the sentinel
but still need to engage the reconnect logic.

If this leads to a futile game of whack-a-mole, we'll obviously need to
try a different approach. But if we do more-or-less build on what I've
got so far, we'll definitely need to ensure it agrees with 27 and 28
before spending serious energy on refinement and tests.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v2-v3.diff --]
[-- Type: text/x-patch, Size: 6268 bytes --]

From 7dcccaef52ad4442eabaabe04cd95ffceec048f7 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 9 Mar 2023 06:33:38 -0800
Subject: [PATCH 0/1] *** NOT A PATCH ***

*** BLURB HERE ***

F. Jason Park (1):
  Add conditional erc-server-reconnect-function

 lisp/erc/erc-backend.el | 93 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 2 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index d289df98bab..1030672506e 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -658,6 +658,31 @@ erc--register-connection
   (run-hooks 'erc--server-post-connect-hook)
   (erc-login))
 
+(defvar erc--server-connect-function #'erc--server-propagate-failed-connection
+  "Function called one second after creating a server process.
+Called with the newly created process just before the opening IRC
+protocol exchange.")
+
+(defun erc--server-propagate-failed-connection (process)
+  "Ensure the PROCESS sentinel runs at least once on early failure.
+Act as a watchdog timer to force `erc-process-sentinel' and its
+finalizers, like `erc-disconnected-hook', to run when PROCESS has
+a status of `failed' after one second.  Print the
+`process-exit-status', which can be a number, like 111, or a
+message, like \"TLS negotiation failed\"."
+  (when (eq (process-status process) 'failed)
+    (erc-display-message
+     nil 'error (process-buffer process)
+     (format "Process exit status: %S" (process-exit-status process)))
+    (pcase (process-exit-status process)
+      ((guard erc--server-reconnect-timer))
+      (111
+       (delete-process process))
+      (`(file-error ,(rx "failed") ,(rx "unreachable") . ,_)
+       (erc-process-sentinel process "failed with code -523\n"))
+      ((rx "tls" (+ nonl) "failed")
+       (erc-process-sentinel process "failed with code -525\n")))))
+
 (defvar erc--server-connect-dumb-ipv6-regexp
   ;; Not for validation (gives false positives).
   (rx bot "[" (group (+ (any xdigit digit ":.")) (? "%" (+ alnum))) "]" eot))
@@ -708,9 +733,11 @@ erc-server-connect
                    (with-current-buffer buffer (erc-current-nick))))
     ;; wait with script loading until we receive a confirmation (first
     ;; MOTD line)
-    (if (eq (process-status process) 'connect)
+    (if (process-contact process :nowait)
         ;; waiting for a non-blocking connect - keep the user informed
-        (erc-display-message nil nil buffer "Opening connection..\n")
+        (progn
+          (erc-display-message nil nil buffer "Opening connection..\n")
+          (run-at-time 1 nil erc--server-connect-function process))
       (message "%s...done" msg)
       (erc--register-connection))))
 
@@ -759,9 +786,9 @@ erc-server-delayed-check-reconnect
                       2)))
     (let ((reschedule
            (lambda (proc)
-             (let ((erc-server-reconnect-timeout
-                    erc--server-reconnect-timeout))
-               (with-current-buffer buffer
+             (with-current-buffer buffer
+               (let ((erc-server-reconnect-timeout
+                      erc--server-reconnect-timeout))
                  (delete-process proc)
                  (erc-display-message nil 'error buffer "Nobody home...")
                  (erc-schedule-reconnect buffer 0))))))
@@ -770,18 +797,22 @@ erc-server-delayed-check-reconnect
                                "*erc-connectivity-check" nil
                                erc-session-server erc-session-port
                                :nowait t))
-                tls-check)
-            (when (and (not (eq erc-session-connector
-                                #'erc-open-network-stream))
-                       (process-contact proc :tls-parameters))
-              (setq tls-check
-                    (run-at-time
-                     1 1 (lambda (proc)
-                           (unless (eq 'connect (process-status proc))
-                             (cancel-timer tls-check))
-                           (when (eq 'failed (process-status proc))
-                             (funcall reschedule proc)))
-                     proc)))
+                (check-expire (time-add 10 (current-time)))
+                check-aside)
+            (setq check-aside
+                  (run-at-time
+                   1 1 (lambda (proc)
+                         (when (or (not (eq 'connect (process-status proc)))
+                                   (time-less-p check-expire (current-time)))
+                           (cancel-timer check-aside))
+                         (when (time-less-p check-expire (current-time))
+                           (erc-display-message
+                            nil 'error buffer "Timed out waiting on `connect'")
+                           (delete-process proc)
+                           (funcall reschedule proc))
+                         (when (eq 'failed (process-status proc))
+                           (funcall reschedule proc)))
+                   proc))
             (set-process-filter
              proc (lambda (proc _)
                     (delete-process proc)
@@ -790,14 +821,16 @@ erc-server-delayed-check-reconnect
                     (run-at-time nil nil #'erc-server-delayed-reconnect
                                  buffer)))
             (set-process-sentinel
-             proc (lambda (cproc event)
+             proc (lambda (proc event)
                     (with-current-buffer buffer
                       (pcase event
                         ("open\n"
-                         (run-at-time nil nil #'send-string
-                                      cproc "PING *connect-check*\r\n"))
-                        ("connection broken by remote peer\n"
-                         (funcall reschedule cproc)))))))
+                         (run-at-time
+                          nil nil #'send-string proc
+                          (format "PING %d\r\n" (time-convert nil 'integer))))
+                        ((or "connection broken by remote peer\n"
+                             (rx bot "failed"))
+                         (funcall reschedule proc)))))))
         (file-error (funcall reschedule nil))))))
 
 (defun erc-server-filter-function (process string)
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Add-conditional-erc-server-reconnect-function.patch --]
[-- Type: text/x-patch, Size: 6530 bytes --]

From 7dcccaef52ad4442eabaabe04cd95ffceec048f7 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Wed, 8 Mar 2023 06:14:36 -0800
Subject: [PATCH 1/1] Add conditional erc-server-reconnect-function

* lisp/erc/erc-backend.el (erc--server-reconnect-timer,
erc-server-delayed-check-reconnect): Add possible alternate value for
option `erc-server-reconnect-function' that only attempts to reconnect
after hearing back from the server.  Also add helper variable.
(erc-server-connect): Run erc--server-connect-function for async
processes.
(erc--server-connect-function,
erc--server-propagate-failed-connection): Add connect-function
variable and default value to run on process creation and execute
process-failure handlers.
---
 lisp/erc/erc-backend.el | 93 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 2 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index 567443f5329..1030672506e 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -658,6 +658,31 @@ erc--register-connection
   (run-hooks 'erc--server-post-connect-hook)
   (erc-login))
 
+(defvar erc--server-connect-function #'erc--server-propagate-failed-connection
+  "Function called one second after creating a server process.
+Called with the newly created process just before the opening IRC
+protocol exchange.")
+
+(defun erc--server-propagate-failed-connection (process)
+  "Ensure the PROCESS sentinel runs at least once on early failure.
+Act as a watchdog timer to force `erc-process-sentinel' and its
+finalizers, like `erc-disconnected-hook', to run when PROCESS has
+a status of `failed' after one second.  Print the
+`process-exit-status', which can be a number, like 111, or a
+message, like \"TLS negotiation failed\"."
+  (when (eq (process-status process) 'failed)
+    (erc-display-message
+     nil 'error (process-buffer process)
+     (format "Process exit status: %S" (process-exit-status process)))
+    (pcase (process-exit-status process)
+      ((guard erc--server-reconnect-timer))
+      (111
+       (delete-process process))
+      (`(file-error ,(rx "failed") ,(rx "unreachable") . ,_)
+       (erc-process-sentinel process "failed with code -523\n"))
+      ((rx "tls" (+ nonl) "failed")
+       (erc-process-sentinel process "failed with code -525\n")))))
+
 (defvar erc--server-connect-dumb-ipv6-regexp
   ;; Not for validation (gives false positives).
   (rx bot "[" (group (+ (any xdigit digit ":.")) (? "%" (+ alnum))) "]" eot))
@@ -708,9 +733,11 @@ erc-server-connect
                    (with-current-buffer buffer (erc-current-nick))))
     ;; wait with script loading until we receive a confirmation (first
     ;; MOTD line)
-    (if (eq (process-status process) 'connect)
+    (if (process-contact process :nowait)
         ;; waiting for a non-blocking connect - keep the user informed
-        (erc-display-message nil nil buffer "Opening connection..\n")
+        (progn
+          (erc-display-message nil nil buffer "Opening connection..\n")
+          (run-at-time 1 nil erc--server-connect-function process))
       (message "%s...done" msg)
       (erc--register-connection))))
 
@@ -744,6 +771,68 @@ erc-server-delayed-reconnect
     (with-current-buffer buffer
       (erc-server-reconnect))))
 
+(defvar-local erc--server-reconnect-timeout nil)
+
+(defun erc-server-delayed-check-reconnect (buffer)
+  "Wait for internet connectivity before trying to reconnect.
+BUFFER is the server buffer for the current connection."
+  ;; This may appear to hang for a good while at various places
+  ;; because it calls wait_reading_process_output a bunch.  It does at
+  ;; least sometimes print "Waiting for socket from ..." or similar.
+  (with-current-buffer buffer
+    (setq erc--server-reconnect-timeout
+          (min 300 (* (or erc--server-reconnect-timeout
+                          erc-server-reconnect-timeout)
+                      2)))
+    (let ((reschedule
+           (lambda (proc)
+             (with-current-buffer buffer
+               (let ((erc-server-reconnect-timeout
+                      erc--server-reconnect-timeout))
+                 (delete-process proc)
+                 (erc-display-message nil 'error buffer "Nobody home...")
+                 (erc-schedule-reconnect buffer 0))))))
+      (condition-case _
+          (let ((proc (funcall erc-session-connector
+                               "*erc-connectivity-check" nil
+                               erc-session-server erc-session-port
+                               :nowait t))
+                (check-expire (time-add 10 (current-time)))
+                check-aside)
+            (setq check-aside
+                  (run-at-time
+                   1 1 (lambda (proc)
+                         (when (or (not (eq 'connect (process-status proc)))
+                                   (time-less-p check-expire (current-time)))
+                           (cancel-timer check-aside))
+                         (when (time-less-p check-expire (current-time))
+                           (erc-display-message
+                            nil 'error buffer "Timed out waiting on `connect'")
+                           (delete-process proc)
+                           (funcall reschedule proc))
+                         (when (eq 'failed (process-status proc))
+                           (funcall reschedule proc)))
+                   proc))
+            (set-process-filter
+             proc (lambda (proc _)
+                    (delete-process proc)
+                    (with-current-buffer buffer
+                      (setq erc--server-reconnect-timeout nil))
+                    (run-at-time nil nil #'erc-server-delayed-reconnect
+                                 buffer)))
+            (set-process-sentinel
+             proc (lambda (proc event)
+                    (with-current-buffer buffer
+                      (pcase event
+                        ("open\n"
+                         (run-at-time
+                          nil nil #'send-string proc
+                          (format "PING %d\r\n" (time-convert nil 'integer))))
+                        ((or "connection broken by remote peer\n"
+                             (rx bot "failed"))
+                         (funcall reschedule proc)))))))
+        (file-error (funcall reschedule nil))))))
+
 (defun erc-server-filter-function (process string)
   "The process filter for the ERC server."
   (with-current-buffer (process-buffer process)
-- 
2.39.2