bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Eric Abrahamsen]

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


Bob Newell requested the ability to customize the
`nnimap-keepalive-timer', and I've opened this bug report with a patch
that adds a new `nnimap-keepalive-times' customization option.

It lets you customize both function interval and period of inactivity,
or you can set it to nil to disable the keepalive altogether.

I also sneaked in a little extra change: since we provided the option to
use Dbus to close Gnus servers when your machine is going to sleep, I've
noticed that sometimes (more often than you'd think) I seem to be
sleeping the machine in between sending the keepalive NOOP and receiving
the response.

When the computer wakes, Emacs is hung while Gnus waits on an OK that
isn't coming, and I need to C-g to get out of it. I would have thought
this was a pretty small target to hit, particular since my imap servers
are all on this computer, but it happens quite often. So this patch also
lets `nnimap-streaming' to t around the keepalive command, so we don't
wait for the response. Semantically incorrect, but the effect is right.

If this is acceptable I can add a NEWS entry (and maybe the manual as
well?).

Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: nnimap-keepalive-customization.diff --]
[-- Type: text/x-patch, Size: 2341 bytes --]

diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index 93e1c47be7..7017bf192e 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -156,6 +156,24 @@ nnimap-split-download-body
   :version "28.1"
   :type 'boolean)
 
+(defcustom nnimap-keepalive-times (cons (* 60 15)
+                                        (* 5 60))
+  "Configuration for the nnimap keepalive timer.
+The value is a cons of two integers (each representing a number
+of seconds): the first is how often to run the keepalive
+function, the second is the seconds of inactivity required to
+send the actual keepalive command.
+
+For example, the default (900 . 300) means to run the check every
+fifteen minutes and, if the user has been inactive for five
+minutes, send the keepalive command.
+
+Set to nil to disable keepalive commands altogether."
+  :version "28.1"
+  :type '(choice (const :tag "disable" nil)
+                 (cons (integer :tag "timer interval")
+                       (integer :tag "seconds of inactivity"))))
+
 (defvar nnimap--split-download-body nil
   "Like `nnimap-split-download-body', but for internal use.")
 
@@ -405,15 +423,16 @@ nnimap-credentials
       nil)))
 
 (defun nnimap-keepalive ()
-  (let ((now (current-time)))
+  (let ((now (current-time))
+        ;; Set this so we don't wait for a response.
+        (nnimap-streaming t))
     (dolist (buffer nnimap-process-buffers)
       (when (buffer-live-p buffer)
 	(with-current-buffer buffer
 	  (when (and nnimap-object
 		     (nnimap-last-command-time nnimap-object)
 		     (time-less-p
-		      ;; More than five minutes since the last command.
-		      (* 5 60)
+		      (cdr nnimap-keepalive-times)
 		      (time-subtract
 		       now
 		       (nnimap-last-command-time nnimap-object))))
@@ -447,8 +466,10 @@ nnimap-map-port
     port))
 
 (defun nnimap-open-connection-1 (buffer)
-  (unless nnimap-keepalive-timer
-    (setq nnimap-keepalive-timer (run-at-time (* 60 15) (* 60 15)
+  (unless (or nnimap-keepalive-timer
+              (null nnimap-keepalive-times))
+    (setq nnimap-keepalive-timer (run-at-time (car nnimap-keepalive-times)
+                                              (car nnimap-keepalive-times)
 					      #'nnimap-keepalive)))
   (with-current-buffer (nnimap-make-process-buffer buffer)
     (let* ((coding-system-for-read 'binary)

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Eric Abrahamsen]

A gentle bump. Lars, do you have any thoughts on this? In the interim
I've started to dislike `nnimap-keepalives-times' as the option name,
but haven't come up with anything better.

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Bob Newell requested the ability to customize the
> `nnimap-keepalive-timer', and I've opened this bug report with a patch
> that adds a new `nnimap-keepalive-times' customization option.
>
> It lets you customize both function interval and period of inactivity,
> or you can set it to nil to disable the keepalive altogether.
>
> I also sneaked in a little extra change: since we provided the option to
> use Dbus to close Gnus servers when your machine is going to sleep, I've
> noticed that sometimes (more often than you'd think) I seem to be
> sleeping the machine in between sending the keepalive NOOP and receiving
> the response.
>
> When the computer wakes, Emacs is hung while Gnus waits on an OK that
> isn't coming, and I need to C-g to get out of it. I would have thought
> this was a pretty small target to hit, particular since my imap servers
> are all on this computer, but it happens quite often. So this patch also
> lets `nnimap-streaming' to t around the keepalive command, so we don't
> wait for the response. Semantically incorrect, but the effect is right.
>
> If this is acceptable I can add a NEWS entry (and maybe the manual as
> well?).
>
> Eric

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Eric Abrahamsen]

A gentle bump. Lars, do you have any thoughts on this? In the interim
I've started to dislike `nnimap-keepalives-times' as the option name,
but haven't come up with anything better.

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Bob Newell requested the ability to customize the
> `nnimap-keepalive-timer', and I've opened this bug report with a patch
> that adds a new `nnimap-keepalive-times' customization option.
>
> It lets you customize both function interval and period of inactivity,
> or you can set it to nil to disable the keepalive altogether.
>
> I also sneaked in a little extra change: since we provided the option to
> use Dbus to close Gnus servers when your machine is going to sleep, I've
> noticed that sometimes (more often than you'd think) I seem to be
> sleeping the machine in between sending the keepalive NOOP and receiving
> the response.
>
> When the computer wakes, Emacs is hung while Gnus waits on an OK that
> isn't coming, and I need to C-g to get out of it. I would have thought
> this was a pretty small target to hit, particular since my imap servers
> are all on this computer, but it happens quite often. So this patch also
> lets `nnimap-streaming' to t around the keepalive command, so we don't
> wait for the response. Semantically incorrect, but the effect is right.
>
> If this is acceptable I can add a NEWS entry (and maybe the manual as
> well?).
>
> Eric

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Bob Newell requested the ability to customize the
> `nnimap-keepalive-timer', and I've opened this bug report with a patch
> that adds a new `nnimap-keepalive-times' customization option.
>
> It lets you customize both function interval and period of inactivity,
> or you can set it to nil to disable the keepalive altogether.

Sure; makes sense.

> I also sneaked in a little extra change: since we provided the option to
> use Dbus to close Gnus servers when your machine is going to sleep, I've
> noticed that sometimes (more often than you'd think) I seem to be
> sleeping the machine in between sending the keepalive NOOP and receiving
> the response.

I think that should be OK -- when we're looking for responses, we use
tags, so the response from the NOOP shouldn't confuse anything.
(Hopefully.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Bob Newell requested the ability to customize the
>> `nnimap-keepalive-timer', and I've opened this bug report with a patch
>> that adds a new `nnimap-keepalive-times' customization option.
>>
>> It lets you customize both function interval and period of inactivity,
>> or you can set it to nil to disable the keepalive altogether.
>
> Sure; makes sense.

Cool. In the meantime I've come to prefer the name
`nnimap-keepalive-intervals', if that's all right.

I'll add this to the manual, as well. Do you think it should be a
defvoo, or a regular customization option?

>> I also sneaked in a little extra change: since we provided the option to
>> use Dbus to close Gnus servers when your machine is going to sleep, I've
>> noticed that sometimes (more often than you'd think) I seem to be
>> sleeping the machine in between sending the keepalive NOOP and receiving
>> the response.
>
> I think that should be OK -- when we're looking for responses, we use
> tags, so the response from the NOOP shouldn't confuse anything.
> (Hopefully.)

Hopefully!

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Cool. In the meantime I've come to prefer the name
> `nnimap-keepalive-intervals', if that's all right.

Yes, even better.

> I'll add this to the manual, as well. Do you think it should be a
> defvoo, or a regular customization option?

Probably a defvoo -- it's possible that people would want different
timeouts for different servers.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Eric Abrahamsen]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Cool. In the meantime I've come to prefer the name
>> `nnimap-keepalive-intervals', if that's all right.
>
> Yes, even better.
>
>> I'll add this to the manual, as well. Do you think it should be a
>> defvoo, or a regular customization option?
>
> Probably a defvoo -- it's possible that people would want different
> timeouts for different servers.

Cool, here's what it looks like now.

Eric


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-new-defvoo-nnimap-keepalive-intervals-to-Gnus-nn.patch --]
[-- Type: text/x-patch, Size: 3811 bytes --]

From 6cd184737b8f9318d5a2d2115721b8771436bd13 Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Mon, 3 May 2021 09:14:24 -0700
Subject: [PATCH] Add new defvoo nnimap-keepalive-intervals to Gnus nnimap
 servers

* lisp/gnus/nnimap.el (nnimap-keepalive-intervals): New per-server
config for customizing when keepalive commands are sent.
(nnimap-keepalive, nnimap-open-connection-1): Consult in these
places.  Additionally, use nnimap-streaming -> t when sending the
keepalive NOOP, so we don't wait for the response.
* doc/misc/gnus.texi (Customizing the IMAP Connection): Document.
---
 doc/misc/gnus.texi  | 10 ++++++++++
 lisp/gnus/nnimap.el | 26 ++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index 869bb27266..71304dc008 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -14515,6 +14515,16 @@ Customizing the IMAP Connection
 and @samp{INBOX.Lists.emacs}, but you'd like the nnimap group names to
 be @samp{INBOX} and @samp{Lists.emacs}, you should enable this option.
 
+@item nnimap-keepalive-intervals
+By default, nnimap will send occasional ``NOOP'' signals to the
+server, to keep the connection alive.  This option governs how often
+that happens.  It is a cons of two integers, representing seconds:
+first how often to run the keepalive check, and the second how many
+seconds of user inactivity are required to actually send the command.
+The default, @code{(900 . 300)}, means run the check every fifteen
+minutes and, if the user has been inactive for five minutes, send the
+keepalive command.  Set to nil to disable keepalive commands altogether.
+
 @end table
 
 
diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index 8990b2bebe..570be49094 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -136,6 +136,16 @@ nnimap-fetch-partial-articles
 likely value would be \"text/\" to automatically fetch all
 textual parts.")
 
+(defvoo nnimap-keepalive-intervals (cons (* 60 15)
+                                         (* 60 5))
+  "Configuration for the nnimap keepalive timer.
+The value is a cons of two integers (each representing a number
+of seconds): the first is how often to run the keepalive
+function, the second is the seconds of inactivity required to
+send the actual keepalive command.
+
+Set to nil to disable keepalive commands altogether.")
+
 (defgroup nnimap nil
   "IMAP for Gnus."
   :group 'gnus)
@@ -405,15 +415,16 @@ nnimap-credentials
       nil)))
 
 (defun nnimap-keepalive ()
-  (let ((now (current-time)))
+  (let ((now (current-time))
+        ;; Set this so we don't wait for a response.
+        (nnimap-streaming t))
     (dolist (buffer nnimap-process-buffers)
       (when (buffer-live-p buffer)
 	(with-current-buffer buffer
 	  (when (and nnimap-object
 		     (nnimap-last-command-time nnimap-object)
 		     (time-less-p
-		      ;; More than five minutes since the last command.
-		      (* 5 60)
+		      (cdr nnimap-keepalive-intervals)
 		      (time-subtract
 		       now
 		       (nnimap-last-command-time nnimap-object))))
@@ -448,9 +459,12 @@ nnimap-map-port
     port))
 
 (defun nnimap-open-connection-1 (buffer)
-  (unless nnimap-keepalive-timer
-    (setq nnimap-keepalive-timer (run-at-time (* 60 15) (* 60 15)
-					      #'nnimap-keepalive)))
+  (unless (or nnimap-keepalive-timer
+              (null nnimap-keepalive-intervals))
+    (setq nnimap-keepalive-timer (run-at-time
+                                  (car nnimap-keepalive-intervals)
+                                  (car nnimap-keepalive-intervals)
+				  #'nnimap-keepalive)))
   (with-current-buffer (nnimap-make-process-buffer buffer)
     (let* ((coding-system-for-read 'binary)
 	   (coding-system-for-write 'binary)
-- 
2.31.1

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> +By default, nnimap will send occasional ``NOOP'' signals to the
> +server, to keep the connection alive.

Perhaps this should say "sends a @samp{NOOP} (keepalive) command" to be
less obscure?

> +keepalive command.  Set to nil to disable keepalive commands altogether.

@code{nil}

Otherwise, looks good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#47478: 28.0.50; Provide customization option for nnimap-keepalive-timer

[Eric Abrahamsen]

On 05/04/21 10:01 AM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> +By default, nnimap will send occasional ``NOOP'' signals to the
>> +server, to keep the connection alive.
>
> Perhaps this should say "sends a @samp{NOOP} (keepalive) command" to be
> less obscure?
>
>> +keepalive command.  Set to nil to disable keepalive commands altogether.
>
> @code{nil}
>
> Otherwise, looks good to me.

Thanks! Done and closing.