bug#43682: 28.0.50; Clean up nnimap server buffers?

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Eric Abrahamsen]

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


Someone noted on gnus.general that their imap connections are frequently
broken, and they end up with a lot of dead process buffers.

I'm talking to them about maybe making the keepalive timeout
configurable, but wouldn't also be tidy to clean up dead process
buffers? How does the attached patch look?

Eric


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

diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index d797e893f5..7f2ebe279e 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -407,8 +407,15 @@ nnimap-keepalive
 		      (time-subtract
 		       now
 		       (nnimap-last-command-time nnimap-object))))
-            (ignore-errors              ;E.g. "buffer foo has no process".
-              (nnimap-send-command "NOOP"))))))))
+	    (condition-case err
+		(process-send-string "NOOP")
+	      (error
+	       (if (string-search "has no process" (cdr err))
+		   (let ((buf (current-buffer)))
+		     (setq nnimap-process-buffers
+			   (delq buf nnimap-process-buffers))
+		     (kill-buffer buf))
+		 (signal (car err) (cdr err)))))))))))
 
 (defun nnimap-open-connection (buffer)
   ;; Be backwards-compatible -- the earlier value of nnimap-stream was
@@ -1910,6 +1917,10 @@ nnimap-find-connection
 		     '(open run)))
 	  (get-buffer-process (cadr entry))
 	(setq nnimap-connection-alist (delq entry nnimap-connection-alist))
+	(setq nnimap-process-buffers
+	      (delq (cadr entry) nnimap-process-buffers))
+	(when (buffer-live-p (cadr entry))
+	  (kill-buffer (cadr entry)))
 	nil))))
 
 ;; Leave room for `open-network-stream' to issue a couple of IMAP

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Robert Pluim]

>>>>> On Mon, 28 Sep 2020 16:37:15 -0700, Eric Abrahamsen <eric@ericabrahamsen.net> said:

    Eric> Someone noted on gnus.general that their imap connections are frequently
    Eric> broken, and they end up with a lot of dead process buffers.

    Eric> I'm talking to them about maybe making the keepalive timeout
    Eric> configurable, but wouldn't also be tidy to clean up dead process
    Eric> buffers? How does the attached patch look?

    Eric> Eric

    Eric> diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
    Eric> index d797e893f5..7f2ebe279e 100644
    Eric> --- a/lisp/gnus/nnimap.el
    Eric> +++ b/lisp/gnus/nnimap.el
    Eric> @@ -407,8 +407,15 @@ nnimap-keepalive
    Eric>  		      (time-subtract
    Eric>  		       now
    Eric>  		       (nnimap-last-command-time nnimap-object))))
    Eric> -            (ignore-errors              ;E.g. "buffer foo has no process".
    Eric> -              (nnimap-send-command "NOOP"))))))))
    Eric> +	    (condition-case err
    Eric> +		(process-send-string "NOOP")
    Eric> +	      (error
    Eric> +	       (if (string-search "has no process" (cdr err))
    Eric> +		   (let ((buf (current-buffer)))
    Eric> +		     (setq nnimap-process-buffers
    Eric> +			   (delq buf nnimap-process-buffers))
    Eric> +		     (kill-buffer buf))
    Eric> +		 (signal (car err) (cdr err)))))))))))

Thatʼs not how you call process-send-string, and the
nnimap-send-command is there for a reason: it deals with imap sequence
numbers.

Also I donʼt think 'has no process' is the only error
'process-send-string' can signal. I see at least 'not running' and
'Output file descriptor.*is closed'

Robert
-- 

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> -            (ignore-errors              ;E.g. "buffer foo has no process".
> -              (nnimap-send-command "NOOP"))))))))
> +	    (condition-case err
> +		(process-send-string "NOOP")
> +	      (error

As Robert notes, you can't do that; it'll mess up the machiner.

> +	       (if (string-search "has no process" (cdr err))
> +		   (let ((buf (current-buffer)))
> +		     (setq nnimap-process-buffers
> +			   (delq buf nnimap-process-buffers))
> +		     (kill-buffer buf))
> +		 (signal (car err) (cdr err)))))))))))

But why look for a string?  You can just check whether the process is
dead or not.

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

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> -            (ignore-errors              ;E.g. "buffer foo has no process".
>> -              (nnimap-send-command "NOOP"))))))))
>> +	    (condition-case err
>> +		(process-send-string "NOOP")
>> +	      (error
>
> As Robert notes, you can't do that; it'll mess up the machiner.

That was a complete brain-o. I must have actually gone and typed that,
and I don't know why.

>> +	       (if (string-search "has no process" (cdr err))
>> +		   (let ((buf (current-buffer)))
>> +		     (setq nnimap-process-buffers
>> +			   (delq buf nnimap-process-buffers))
>> +		     (kill-buffer buf))
>> +		 (signal (car err) (cdr err)))))))))))
>
> But why look for a string?  You can just check whether the process is
> dead or not.

Yes, I could have thought this through a little more...

Do we need to manipulate `nnimap-connection-alist', as
`nnimap-find-connection' does? I've never understood what that variable
is actually for. It's a defvoo, so it has a separate value per-server,
but each server's only got one active process buffer anyway. What does
it tell us that we can't get from nnimap-process-buffers (and those
buffers' local values of `nnimap-object')?

Thanks to you both,
Eric

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Do we need to manipulate `nnimap-connection-alist', as
> `nnimap-find-connection' does? I've never understood what that variable
> is actually for. It's a defvoo, so it has a separate value per-server,
> but each server's only got one active process buffer anyway.

They do?  I thought async prefetching opened a second connection.  Is
that in nnimap only?

And...  was there something about sieve-mode having its own connection?
I forget.

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

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> They do?  I thought async prefetching opened a second connection.  Is
> that in nnimap only?

Sorry; I meant "nntp only".

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

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Eric Abrahamsen]

On 09/30/20 03:18 AM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Do we need to manipulate `nnimap-connection-alist', as
>> `nnimap-find-connection' does? I've never understood what that variable
>> is actually for. It's a defvoo, so it has a separate value per-server,
>> but each server's only got one active process buffer anyway.
>
> They do?  I thought async prefetching opened a second connection.  Is
> that in nnimap only?

So that's what it's for! That's a mystery solved. It appears that only
nntp is asynchronous: at least, it's the only backend that implements
`gnus-asynchronous-p'.

nntp.el also contains `nntp-async-{wait,stop-trigger}', but as far as I
can tell that has nothing to do with gnus-async.el stuff (?). That's the
code that appears to make use of "extra processes per server", and
nnimap doesn't have any of that.

> And... was there something about sieve-mode having its own connection?
> I forget.

Not so far as I can tell. sieve.el and sieve-mode.el don't have anything
to do with Gnus, and gnus-sieve.el is just about building sieve scripts
from Gnus splitting rules. (Now that you mention it, it would be nice if
there were a command to edit an IMAP server's sieve scripts from the
*Server* buffer.)

I went back through nnimap.el as carefully as I could. It's
`nnimap-find-connection' and `nnimap-open-connection' that end up making
use of `nnimap-connection-alist', but both of them only ever use
`nntp-server-buffer' as the alist key. Unless that's been sneakily
let-bound to something different someplace, the
`nnimap-connection-alist' doesn't appear to do anything.

Also, `nnimap-make-process-buffer' sets a buffer-local version of
`after-change-functions' to nil -- this looks a bit like how nntp
handles its async code, but it isn't used in nnimap. My guess is that
someone set out to implement async for nnimap, but never quite finished
it.

I'm inclined to delete the alist altogether, but if we keep it at least
I can give it a docstring.

Eric

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> So that's what it's for! That's a mystery solved. It appears that only
> nntp is asynchronous: at least, it's the only backend that implements
> `gnus-asynchronous-p'.

Aha.  nnimap should, too.

> nntp.el also contains `nntp-async-{wait,stop-trigger}', but as far as I
> can tell that has nothing to do with gnus-async.el stuff (?). That's the
> code that appears to make use of "extra processes per server", and
> nnimap doesn't have any of that.

Yes, that's a separate thing...

> Not so far as I can tell. sieve.el and sieve-mode.el don't have anything
> to do with Gnus, and gnus-sieve.el is just about building sieve scripts
> from Gnus splitting rules. (Now that you mention it, it would be nice if
> there were a command to edit an IMAP server's sieve scripts from the
> *Server* buffer.)

Right.  I think there was once some talk about this stuff, but oy vey
the years...

> I'm inclined to delete the alist altogether, but if we keep it at least
> I can give it a docstring.

No, we should keep it and implement the stuff for gnus-asynchronous-p in
nnimap, too.

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

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Eric Abrahamsen]

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


On 10/01/20 03:30 AM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> So that's what it's for! That's a mystery solved. It appears that only
>> nntp is asynchronous: at least, it's the only backend that implements
>> `gnus-asynchronous-p'.
>
> Aha.  nnimap should, too.
>
>> nntp.el also contains `nntp-async-{wait,stop-trigger}', but as far as I
>> can tell that has nothing to do with gnus-async.el stuff (?). That's the
>> code that appears to make use of "extra processes per server", and
>> nnimap doesn't have any of that.
>
> Yes, that's a separate thing...
>
>> Not so far as I can tell. sieve.el and sieve-mode.el don't have anything
>> to do with Gnus, and gnus-sieve.el is just about building sieve scripts
>> from Gnus splitting rules. (Now that you mention it, it would be nice if
>> there were a command to edit an IMAP server's sieve scripts from the
>> *Server* buffer.)
>
> Right.  I think there was once some talk about this stuff, but oy vey
> the years...

I'll add it to my own todo list, where it can likewise languish for
years.

>> I'm inclined to delete the alist altogether, but if we keep it at least
>> I can give it a docstring.
>
> No, we should keep it and implement the stuff for gnus-asynchronous-p in
> nnimap, too.

It's possible (I'm not claiming to understand all the code) that all we
would need to do is fix `gnus-async-wait-for-article' to replace its
calls to `nntp-find-connection' and `nntp-accept-process-output' with
something generalized. Those two functions deal with directly with
`nntp-connection-alist', so we'd need something that would do the
equivalent with `nnimap-connection-alist'.

Anyway, in the interest of completing this far less ambitious patch: if
the nnimap connection has timed out, we should remove this connection
from `nnimap-connection-alist', so this version of the patch does that.
If async has opened a second connection, I guess we should leave that
alone, though I don't have too much confidence that the whole process
will recover gracefully from the main connection dying...

Eric


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

diff --git a/lisp/gnus/nnimap.el b/lisp/gnus/nnimap.el
index d797e893f5..bceba574ca 100644
--- a/lisp/gnus/nnimap.el
+++ b/lisp/gnus/nnimap.el
@@ -407,8 +407,19 @@ nnimap-keepalive
 		      (time-subtract
 		       now
 		       (nnimap-last-command-time nnimap-object))))
-            (ignore-errors              ;E.g. "buffer foo has no process".
-              (nnimap-send-command "NOOP"))))))))
+	    (ignore-errors
+	      (nnimap-send-command "NOOP"))
+	    ;; If our connection has died in the meantime, clean it
+	    ;; and its buffer up.
+	    (unless (memq (process-status (get-buffer-process buffer))
+			  '(open run))
+	      (setq nnimap-process-buffers
+		    (delq buffer nnimap-process-buffers))
+	      (setq nnimap-connection-alist
+		    (seq-filter (lambda (elt)
+				  (null (eq buffer (cdr elt)))
+				  nnimap-connection-alist)))
+	      (kill-buffer buffer))))))))
 
 (defun nnimap-open-connection (buffer)
   ;; Be backwards-compatible -- the earlier value of nnimap-stream was
@@ -1910,6 +1921,10 @@ nnimap-find-connection
 		     '(open run)))
 	  (get-buffer-process (cadr entry))
 	(setq nnimap-connection-alist (delq entry nnimap-connection-alist))
+	(setq nnimap-process-buffers
+	      (delq (cadr entry) nnimap-process-buffers))
+	(when (buffer-live-p (cadr entry))
+	  (kill-buffer (cadr entry)))
 	nil))))
 
 ;; Leave room for `open-network-stream' to issue a couple of IMAP

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> It's possible (I'm not claiming to understand all the code) that all we
> would need to do is fix `gnus-async-wait-for-article' to replace its
> calls to `nntp-find-connection' and `nntp-accept-process-output' with
> something generalized. Those two functions deal with directly with
> `nntp-connection-alist', so we'd need something that would do the
> equivalent with `nnimap-connection-alist'.

Yup.

> Anyway, in the interest of completing this far less ambitious patch: if
> the nnimap connection has timed out, we should remove this connection
> from `nnimap-connection-alist', so this version of the patch does that.
> If async has opened a second connection, I guess we should leave that
> alone, though I don't have too much confidence that the whole process
> will recover gracefully from the main connection dying...

Well, the connections are separate, and there's all kinds of reasons for
the server to close a connection, so...

> +	    (unless (memq (process-status (get-buffer-process buffer))
> +			  '(open run))

Aka `process-live-p'.

Otherwise looks fine to me (but I haven't tested the code).

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

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Eric Abrahamsen]

On 10/01/20 18:01 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> It's possible (I'm not claiming to understand all the code) that all we
>> would need to do is fix `gnus-async-wait-for-article' to replace its
>> calls to `nntp-find-connection' and `nntp-accept-process-output' with
>> something generalized. Those two functions deal with directly with
>> `nntp-connection-alist', so we'd need something that would do the
>> equivalent with `nnimap-connection-alist'.
>
> Yup.

This is something I wouldn't want to tackle until we have generic
functions.

>> Anyway, in the interest of completing this far less ambitious patch: if
>> the nnimap connection has timed out, we should remove this connection
>> from `nnimap-connection-alist', so this version of the patch does that.
>> If async has opened a second connection, I guess we should leave that
>> alone, though I don't have too much confidence that the whole process
>> will recover gracefully from the main connection dying...
>
> Well, the connections are separate, and there's all kinds of reasons for
> the server to close a connection, so...
>
>> +	    (unless (memq (process-status (get-buffer-process buffer))
>> +			  '(open run))
>
> Aka `process-live-p'.

I forgot we have that!

> Otherwise looks fine to me (but I haven't tested the code).

Okay, I'll run this for a bit, as well.

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Stefan Kangas]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 10/01/20 18:01 PM, Lars Ingebrigtsen wrote:
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> It's possible (I'm not claiming to understand all the code) that all we
>>> would need to do is fix `gnus-async-wait-for-article' to replace its
>>> calls to `nntp-find-connection' and `nntp-accept-process-output' with
>>> something generalized. Those two functions deal with directly with
>>> `nntp-connection-alist', so we'd need something that would do the
>>> equivalent with `nnimap-connection-alist'.
>>
>> Yup.
>
> This is something I wouldn't want to tackle until we have generic
> functions.
>
>>> Anyway, in the interest of completing this far less ambitious patch: if
>>> the nnimap connection has timed out, we should remove this connection
>>> from `nnimap-connection-alist', so this version of the patch does that.
>>> If async has opened a second connection, I guess we should leave that
>>> alone, though I don't have too much confidence that the whole process
>>> will recover gracefully from the main connection dying...
>>
>> Well, the connections are separate, and there's all kinds of reasons for
>> the server to close a connection, so...
>>
>>> +	    (unless (memq (process-status (get-buffer-process buffer))
>>> +			  '(open run))
>>
>> Aka `process-live-p'.
>
> I forgot we have that!
>
>> Otherwise looks fine to me (but I haven't tested the code).
>
> Okay, I'll run this for a bit, as well.

Any news here?  Should the fix be installed?

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Eric Abrahamsen]

Stefan Kangas <stefan@marxist.se> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> On 10/01/20 18:01 PM, Lars Ingebrigtsen wrote:
>>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>>
>>>> It's possible (I'm not claiming to understand all the code) that all we
>>>> would need to do is fix `gnus-async-wait-for-article' to replace its
>>>> calls to `nntp-find-connection' and `nntp-accept-process-output' with
>>>> something generalized. Those two functions deal with directly with
>>>> `nntp-connection-alist', so we'd need something that would do the
>>>> equivalent with `nnimap-connection-alist'.
>>>
>>> Yup.
>>
>> This is something I wouldn't want to tackle until we have generic
>> functions.
>>
>>>> Anyway, in the interest of completing this far less ambitious patch: if
>>>> the nnimap connection has timed out, we should remove this connection
>>>> from `nnimap-connection-alist', so this version of the patch does that.
>>>> If async has opened a second connection, I guess we should leave that
>>>> alone, though I don't have too much confidence that the whole process
>>>> will recover gracefully from the main connection dying...
>>>
>>> Well, the connections are separate, and there's all kinds of reasons for
>>> the server to close a connection, so...
>>>
>>>> +	    (unless (memq (process-status (get-buffer-process buffer))
>>>> +			  '(open run))
>>>
>>> Aka `process-live-p'.
>>
>> I forgot we have that!
>>
>>> Otherwise looks fine to me (but I haven't tested the code).
>>
>> Okay, I'll run this for a bit, as well.
>
> Any news here?  Should the fix be installed?

Hmm, I found a couple of typos in it, then I ran it for a bit, then got
distracted and backed it out in favor of something else, and then...

I think it should be okay to push. I'll try a bit of explicit testing,
then push later today. Thanks for the reminder!

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Eric Abrahamsen]

On 10/11/21 05:53 AM, Stefan Kangas wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> On 10/01/20 18:01 PM, Lars Ingebrigtsen wrote:
>>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>>
>>>> It's possible (I'm not claiming to understand all the code) that all we
>>>> would need to do is fix `gnus-async-wait-for-article' to replace its
>>>> calls to `nntp-find-connection' and `nntp-accept-process-output' with
>>>> something generalized. Those two functions deal with directly with
>>>> `nntp-connection-alist', so we'd need something that would do the
>>>> equivalent with `nnimap-connection-alist'.
>>>
>>> Yup.
>>
>> This is something I wouldn't want to tackle until we have generic
>> functions.
>>
>>>> Anyway, in the interest of completing this far less ambitious patch: if
>>>> the nnimap connection has timed out, we should remove this connection
>>>> from `nnimap-connection-alist', so this version of the patch does that.
>>>> If async has opened a second connection, I guess we should leave that
>>>> alone, though I don't have too much confidence that the whole process
>>>> will recover gracefully from the main connection dying...
>>>
>>> Well, the connections are separate, and there's all kinds of reasons for
>>> the server to close a connection, so...
>>>
>>>> +	    (unless (memq (process-status (get-buffer-process buffer))
>>>> +			  '(open run))
>>>
>>> Aka `process-live-p'.
>>
>> I forgot we have that!
>>
>>> Otherwise looks fine to me (but I haven't tested the code).
>>
>> Okay, I'll run this for a bit, as well.
>
> Any news here?  Should the fix be installed?

Okay, I've pushed a commit that fixes nearly all of the issue. There are
still some mysterious circumstances under which a single dead imap
connection buffer remains in `nnimap-process-buffers', and I don't know
how it gets there. But it's better than 50+ dead connection buffers, so
I'm going to close this for now.

bug#43682: 28.0.50; Clean up nnimap server buffers?

[Stefan Kangas]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Okay, I've pushed a commit that fixes nearly all of the issue. There are
> still some mysterious circumstances under which a single dead imap
> connection buffer remains in `nnimap-process-buffers', and I don't know
> how it gets there. But it's better than 50+ dead connection buffers, so
> I'm going to close this for now.

Thanks!