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

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

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


A stop-gap solution for this issue was introduced in ERC 5.6 as
`erc-server-delayed-check-reconnect', a new function value for the
option `erc-server-reconnect-function'. There's since been some
criticism regarding its lack of discoverability, despite it featuring in
the Sample Configuration of ERC's manual. It's therefore been suggested
we favor the behavior it enables whenever possible by detecting
compatible values of `erc-server-connect-function'. Any false negatives
will hopefully only affect advanced users with nonstandard connectors.
The following patch tries to implement this. Feedback welcome.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6.1-Add-smarter-default-for-erc-server-reconnect-f.patch --]
[-- Type: text/x-patch, Size: 17502 bytes --]

From 8383193362db77d814747d9f2074fe514359e76c Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Fri, 29 Nov 2024 15:56:47 -0800
Subject: [PATCH] [5.6.1] Add smarter default for erc-server-reconnect-function

* doc/misc/erc.texi (Sample Configuration): Remove customization in
`use-package' declaration for `erc-server-reconnect-function' as well as
related language in the customization walk-through.  Do this because the
new default incorporates `erc-server-delayed-check-reconnect'.
* etc/ERC-NEWS: Announce new default for `erc-server-reconnect-function'.
* lisp/erc/erc-backend.el (erc-server-reconnect-function): Change
default to `erc-server-prefer-check-reconnect'.
(erc-server-delayed-check-reconnect): Use `process-send-string' instead
of `send-string'.
(erc-server-known-check-aware-connectors): New variable.
(erc-server-prefer-check-reconnect): New function.
* test/lisp/erc/erc-scenarios-base-auto-recon.el
(erc-scenarios-base-auto-recon-unavailable)
(erc-scenarios-base-auto-recon-no-proto): Remove unnecessary
`erc-server-reconnect-function' binding because the new default
still incorporates the behavior under test that's exhibited by
`erc-server-delayed-check-reconnect'.
* test/lisp/erc/erc-scenarios-base-buffer-display.el
(erc-scenarios-base-buffer-display--reconnect-common):
* test/lisp/erc/erc-scenarios-base-compat-rename-bouncer.el
(erc-scenarios-common--base-compat-no-rename-bouncer):
* test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-base.el
(erc-scenarios-base-netid-bouncer--recon-base):
* test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-both.el
(erc-scenarios-base-netid-bouncer--recon-both):
* test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-id.el
(erc-scenarios-base-netid-bouncer--reconnect-id-foo)
(erc-scenarios-base-netid-bouncer--reconnect-id-bar):
* test/lisp/erc/erc-scenarios-base-reconnect.el
(erc-scenarios-base-reconnect-timer)
(erc-scenarios-base-cancel-reconnect):
* test/lisp/erc/erc-scenarios-services-misc.el
(erc-scenarios-services-misc--reconnect-retry-nick):
* test/lisp/erc/erc-scenarios-stamp.el
(erc-scenarios-stamp--date-mode/reconnect): Explicitly bind
`erc-server-reconnect-function' to `erc-server-delayed-reconnect', the
former default.  (Bug#62044)
---
 doc/misc/erc.texi                             | 37 ++++++-------------
 etc/ERC-NEWS                                  |  6 +++
 lisp/erc/erc-backend.el                       | 21 +++++++++--
 .../lisp/erc/erc-scenarios-base-auto-recon.el |  8 ++--
 .../erc/erc-scenarios-base-buffer-display.el  |  1 +
 ...rc-scenarios-base-compat-rename-bouncer.el |  1 +
 ...scenarios-base-netid-bouncer-recon-base.el |  3 +-
 ...scenarios-base-netid-bouncer-recon-both.el |  4 +-
 ...c-scenarios-base-netid-bouncer-recon-id.el |  6 ++-
 test/lisp/erc/erc-scenarios-base-reconnect.el |  2 +
 test/lisp/erc/erc-scenarios-services-misc.el  |  1 +
 test/lisp/erc/erc-scenarios-stamp.el          |  1 +
 12 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/doc/misc/erc.texi b/doc/misc/erc.texi
index 0f6b6b8c5be..d7791c04357 100644
--- a/doc/misc/erc.texi
+++ b/doc/misc/erc.texi
@@ -1340,8 +1340,7 @@ Sample Configuration
   ;; Scroll all windows to prompt when submitting input.
   (erc-scrolltobottom-all t)
 
-  ;; Reconnect automatically using a fancy strategy.
-  (erc-server-reconnect-function #'erc-server-delayed-check-reconnect)
+  ;; Wait a bit longer between automatic reconnect attempts.
   (erc-server-reconnect-timeout 30)
 
   ;; Show new buffers in the current window instead of a split.
@@ -1444,30 +1443,18 @@ Sample Configuration
 finished, hit @kbd{C-x C-s} or click @samp{[Apply and Save]} atop the
 buffer.
 
-Now do the same for another couple options, this time having to do
-with automatic reconnection.  But instead of searching for their print
-names, try running @kbd{M-x customize-option @key{RET} @samp{<option>}
-@key{RET}}, replacing @samp{<option>} with:
+Now do the same for another option, this time having to do with
+automatic reconnection.  But instead of searching for its print name,
+try running @kbd{M-x customize-option @key{RET}
+@samp{erc-server-reconnect-timeout} @key{RET}}.  (If it helps, hit
+@key{TAB} for completion.)  As you may have noticed, when customizing
+options individually, each buffer displays but a single option's widget.
+For @code{erc-server-reconnect-timeout}, you'll encounter a text field
+(instead of a button), which works like those in a typical web form.
+Enter @samp{30} and either hit @kbd{C-x C-s} to save or @key{TAB} over
+to @samp{[State]} and hit @key{RET} followed by @kbd{1} to persists your
+changes.
 
-@itemize @bullet
-@item @code{erc-server-reconnect-function}, a function
-@item @code{erc-server-reconnect-timeout}, a number
-@end itemize
-
-@noindent
-(If it helps, hit @key{TAB} for completion.)  As you may have noticed,
-when customizing options individually, each buffer displays but a
-single option's widget.  When you get to the buffer for ``Erc Server
-Reconnect Function'', you'll see that @samp{[Toggle]} has been
-replaced with @samp{[Value Menu]} and that clicking it reveals three
-choices in a pop-up window.  Enter @kbd{1} to select
-@code{erc-server-delayed-check-reconnect} before @key{TAB}'ing over to
-@samp{[State]} and hitting @key{RET}.  Enter @kbd{1} again, this time
-to persists your changes.
-
-For the final option, @code{erc-server-reconnect-timeout}, you'll
-encounter a text field (instead of a button), which works like those
-in a typical web form.  Enter @samp{30} and hit @kbd{C-x C-s} to save.
 Just for fun, click the group link for @samp{Erc Server} at the bottom
 of the buffer.  You could just as well have set the last two options
 from this ``custom group'' buffer alone, which very much resembles the
diff --git a/etc/ERC-NEWS b/etc/ERC-NEWS
index f3c8645f02d..b7b49956d06 100644
--- a/etc/ERC-NEWS
+++ b/etc/ERC-NEWS
@@ -62,6 +62,12 @@ of concerns and the newer module's "experimental" status, the migration
 was deemed worth any potential disruption, despite this being a point
 release.  ERC appreciates your understanding in this matter.
 
+** Option 'erc-server-reconnect-function' has a new default.
+ERC 5.6 added 'erc-server-delayed-check-reconnect', whose "probing"
+strategy worked better for most users.  While compatibility concerns
+prevented it from becoming the new 'erc-server-reconnect-function'
+outright, a new solution has emerged that defers to it when sensible.
+
 ** Entry-point command 'erc-tls' once again considers option 'erc-port'.
 In its zeal to enforce a preference for TLS connections, ERC 5.5 went a
 bit far in disregarding the useful user option 'erc-port'.  When called
diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index e72fa036f17..0d39f3949f9 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -429,15 +429,16 @@ erc-server-reconnect-timeout
 means of handling this situation on some servers."
   :type 'number)
 
-(defcustom erc-server-reconnect-function 'erc-server-delayed-reconnect
+(defcustom erc-server-reconnect-function 'erc-server-prefer-check-reconnect
   "Function called by the reconnect timer to create a new connection.
 Called with a server buffer as its only argument.  Potential uses
 include exponential backoff and probing for connectivity prior to
 dialing.  Use `erc-schedule-reconnect' to instead try again later
 and optionally alter the attempts tally."
-  :package-version '(ERC . "5.5")
+  :package-version '(ERC . "5.6.1")
   :type '(choice (function-item erc-server-delayed-reconnect)
                  (function-item erc-server-delayed-check-reconnect)
+                 (function-item erc-server-prefer-check-reconnect)
                  function))
 
 (defcustom erc-split-line-length 440
@@ -879,7 +880,7 @@ erc-server-delayed-check-reconnect
              (sentinel (lambda (proc event)
                          (pcase event
                            ("open\n"
-                            (run-at-time nil nil #'send-string proc
+                            (run-at-time nil nil #'process-send-string proc
                                          (format "PING %d\r\n"
                                                  (time-convert nil 'integer))))
                            ((or "connection broken by remote peer\n"
@@ -901,6 +902,20 @@ erc-server-delayed-check-reconnect
               (set-process-sentinel proc sentinel))
           (file-error (funcall reschedule nil)))))))
 
+(defvar erc-server-known-check-aware-connectors
+  '(erc-open-tls-stream erc-open-network-stream)
+  "Functions compatible with `erc-server-delayed-check-reconnect'.")
+
+(defun erc-server-prefer-check-reconnect (buffer)
+  "Defer to another reconnector based on BUFFER's `erc-session-connector'.
+When it's a member of `erc-server-known-check-aware-connectors', prefer
+`erc-server-delayed-check-reconnect'.  Otherwise, use
+`erc-server-delayed-reconnect'."
+  (if (memq (buffer-local-value 'erc-session-connector buffer)
+            erc-server-known-check-aware-connectors)
+      (erc-server-delayed-check-reconnect buffer)
+    (erc-server-delayed-reconnect buffer)))
+
 (defun erc-server-filter-function (process string)
   "The process filter for the ERC server."
   (with-current-buffer (process-buffer process)
diff --git a/test/lisp/erc/erc-scenarios-base-auto-recon.el b/test/lisp/erc/erc-scenarios-base-auto-recon.el
index 808b1d8c4d4..d6a114147b3 100644
--- a/test/lisp/erc/erc-scenarios-base-auto-recon.el
+++ b/test/lisp/erc/erc-scenarios-base-auto-recon.el
@@ -24,6 +24,10 @@
   (let ((load-path (cons (ert-resource-directory) load-path)))
     (require 'erc-scenarios-common)))
 
+;; This tests `erc-server-delayed-check-reconnect', which is called by
+;; `erc-server-prefer-check-reconnect' (the default value of
+;; `erc-server-reconnect-function' as of ERC 5.6.1).
+
 (defun erc-scenarios-base-auto-recon--get-unused-port ()
   (let ((server (make-network-process :name "*erc-scenarios-base-auto-recon*"
                                       :host "localhost"
@@ -42,7 +46,6 @@ erc-scenarios-base-auto-recon-unavailable
        (port (erc-scenarios-base-auto-recon--get-unused-port))
        (erc--server-reconnect-timeout-scale-function (lambda (_) 1))
        (erc-server-auto-reconnect t)
-       (erc-server-reconnect-function #'erc-server-delayed-check-reconnect)
        (expect (erc-d-t-make-expecter))
        (erc-scenarios-common-dialog "base/reconnect")
        (dumb-server nil))
@@ -89,7 +92,7 @@ erc-scenarios-base-auto-recon-unavailable
         (erc-cmd-RECONNECT "cancel")
         (funcall expect 10 "canceled")))))
 
-;; In this test, a listener accepts but doesn't respond to any messages.
+;; Here, a listener accepts but doesn't respond to any messages.
 
 (ert-deftest erc-scenarios-base-auto-recon-no-proto ()
   :tags '(:expensive-test)
@@ -102,7 +105,6 @@ erc-scenarios-base-auto-recon-no-proto
        (erc--server-reconnect-timeout-scale-function (lambda (_) 1))
        (erc--server-reconnect-timeout-check 0.5)
        (erc-server-auto-reconnect t)
-       (erc-server-reconnect-function #'erc-server-delayed-check-reconnect)
        (expect (erc-d-t-make-expecter)))
 
     (ert-info ("Session succeeds but cut short")
diff --git a/test/lisp/erc/erc-scenarios-base-buffer-display.el b/test/lisp/erc/erc-scenarios-base-buffer-display.el
index 5c3c526f86d..137db955db5 100644
--- a/test/lisp/erc/erc-scenarios-base-buffer-display.el
+++ b/test/lisp/erc/erc-scenarios-base-buffer-display.el
@@ -40,6 +40,7 @@ erc-scenarios-base-buffer-display--reconnect-common
        (port (process-contact dumb-server :service))
        (expect (erc-d-t-make-expecter))
        (erc-server-flood-penalty 0.1)
+       (erc-server-reconnect-function #'erc-server-delayed-reconnect)
        (erc-server-auto-reconnect t)
        erc-autojoin-channels-alist)
 
diff --git a/test/lisp/erc/erc-scenarios-base-compat-rename-bouncer.el b/test/lisp/erc/erc-scenarios-base-compat-rename-bouncer.el
index d1124269e0a..0bba0446ce6 100644
--- a/test/lisp/erc/erc-scenarios-base-compat-rename-bouncer.el
+++ b/test/lisp/erc/erc-scenarios-base-compat-rename-bouncer.el
@@ -43,6 +43,7 @@ erc-scenarios-common--base-compat-no-rename-bouncer
        (chan-buf-foo (format "#chan@127.0.0.1:%d" port))
        (chan-buf-bar (format "#chan@127.0.0.1:%d<2>" port))
        (expect (erc-d-t-make-expecter))
+       (erc-server-reconnect-function #'erc-server-delayed-reconnect)
        (erc-server-auto-reconnect auto)
        erc-server-buffer-foo erc-server-process-foo
        erc-server-buffer-bar erc-server-process-bar)
diff --git a/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-base.el b/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-base.el
index e5453c40e56..04ccb86643b 100644
--- a/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-base.el
+++ b/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-base.el
@@ -26,6 +26,7 @@
 
 (ert-deftest erc-scenarios-base-netid-bouncer--recon-base ()
   :tags '(:expensive-test)
-  (erc-scenarios-common--base-network-id-bouncer--reconnect nil nil))
+  (let ((erc-server-reconnect-function #'erc-server-delayed-reconnect))
+    (erc-scenarios-common--base-network-id-bouncer--reconnect nil nil)))
 
 ;;; erc-scenarios-base-netid-bouncer-recon-base.el ends here
diff --git a/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-both.el b/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-both.el
index 09ddcef5fa3..f0939d6204a 100644
--- a/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-both.el
+++ b/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-both.el
@@ -27,6 +27,8 @@
 
 (ert-deftest erc-scenarios-base-netid-bouncer--recon-both ()
   :tags '(:expensive-test)
-  (erc-scenarios-common--base-network-id-bouncer--reconnect 'foo-id 'bar-id))
+  (let ((erc-server-reconnect-function #'erc-server-delayed-reconnect))
+    (erc-scenarios-common--base-network-id-bouncer--reconnect 'foo-id
+                                                              'bar-id)))
 
 ;;; erc-scenarios-base-netid-bouncer-recon-both.el ends here
diff --git a/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-id.el b/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-id.el
index 253ab4f72c9..d77a4287976 100644
--- a/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-id.el
+++ b/test/lisp/erc/erc-scenarios-base-netid-bouncer-recon-id.el
@@ -26,11 +26,13 @@
 
 (ert-deftest erc-scenarios-base-netid-bouncer--reconnect-id-foo ()
   :tags '(:expensive-test)
-  (erc-scenarios-common--base-network-id-bouncer--reconnect 'foo-id nil))
+  (let ((erc-server-reconnect-function #'erc-server-delayed-reconnect))
+    (erc-scenarios-common--base-network-id-bouncer--reconnect 'foo-id nil)))
 
 (ert-deftest erc-scenarios-base-netid-bouncer--reconnect-id-bar ()
   :tags '(:expensive-test)
-  (erc-scenarios-common--base-network-id-bouncer--reconnect nil 'bar-id))
+  (let ((erc-server-reconnect-function #'erc-server-delayed-reconnect))
+    (erc-scenarios-common--base-network-id-bouncer--reconnect nil 'bar-id)))
 
 
 ;;; erc-scenarios-base-netid-bouncer-recon-id.el ends here
diff --git a/test/lisp/erc/erc-scenarios-base-reconnect.el b/test/lisp/erc/erc-scenarios-base-reconnect.el
index 6f968b9fcbc..2dc34362fbf 100644
--- a/test/lisp/erc/erc-scenarios-base-reconnect.el
+++ b/test/lisp/erc/erc-scenarios-base-reconnect.el
@@ -37,6 +37,7 @@ erc-scenarios-base-reconnect-timer
        (dumb-server (erc-d-run "localhost" t 'timer 'timer 'timer-last))
        (port (process-contact dumb-server :service))
        (expect (erc-d-t-make-expecter))
+       (erc-server-reconnect-function #'erc-server-delayed-reconnect)
        (erc-server-auto-reconnect t)
        erc-autojoin-channels-alist
        erc-server-buffer)
@@ -144,6 +145,7 @@ erc-scenarios-base-cancel-reconnect
        (dumb-server (erc-d-run "localhost" t 'timer 'timer 'timer-last))
        (port (process-contact dumb-server :service))
        (expect (erc-d-t-make-expecter))
+       (erc-server-reconnect-function #'erc-server-delayed-reconnect)
        (erc-server-auto-reconnect t)
        erc-autojoin-channels-alist
        erc-server-buffer)
diff --git a/test/lisp/erc/erc-scenarios-services-misc.el b/test/lisp/erc/erc-scenarios-services-misc.el
index 47d0bcff41a..ee35e8b93db 100644
--- a/test/lisp/erc/erc-scenarios-services-misc.el
+++ b/test/lisp/erc/erc-scenarios-services-misc.el
@@ -155,6 +155,7 @@ erc-scenarios-services-misc--reconnect-retry-nick
        (dumb-server (erc-d-run "localhost" t 'reconnect-retry
                                'reconnect-retry-again))
        (port (process-contact dumb-server :service))
+       (erc-server-reconnect-function #'erc-server-delayed-reconnect)
        (erc-server-auto-reconnect t)
        (erc-modules `(services-regain sasl ,@erc-modules))
        (erc-services-regain-alist
diff --git a/test/lisp/erc/erc-scenarios-stamp.el b/test/lisp/erc/erc-scenarios-stamp.el
index 8aea091333b..86074a9c35a 100644
--- a/test/lisp/erc/erc-scenarios-stamp.el
+++ b/test/lisp/erc/erc-scenarios-stamp.el
@@ -188,6 +188,7 @@ erc-scenarios-stamp--date-mode/reconnect
       ((erc-scenarios-common-dialog "base/reconnect")
        (erc-server-flood-penalty 0.1)
        (erc-stamp--tz t)
+       (erc-server-reconnect-function #'erc-server-delayed-reconnect)
        (erc-server-auto-reconnect t)
        ;; Start close to midnight: 2024-06-02T23:58:11.055Z
        (erc-stamp--current-time (if (< emacs-major-version 29)
-- 
2.47.0