bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Andreas Schwab]

When the connection to the nntp server times out (if
nntp-connection-timeout is non-nil) nntp-with-open-group-function kills
the process buffer.  But that is the current buffer at this point if
called from nntp-finish-retrieve-group-infos, causing the latter
function to try to read (and modify!) a random buffer that happend to be
the LRU buffer at that point.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Fri, 14 Apr 2023 23:47:03 +0200
> 
> When the connection to the nntp server times out (if
> nntp-connection-timeout is non-nil) nntp-with-open-group-function kills
> the process buffer.  But that is the current buffer at this point if
> called from nntp-finish-retrieve-group-infos, causing the latter
> function to try to read (and modify!) a random buffer that happend to be
> the LRU buffer at that point.

Thanks.

If this is a recent regression, could you perhaps bisect to find the
offending commit(s)?

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Andreas Schwab]

commit 032969e8c65 "Don't have nntp-report signal an error"

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

Andreas Schwab <schwab@linux-m68k.org> writes:

> commit 032969e8c65 "Don't have nntp-report signal an error"

Ooh, I knew this would end up being me. Give me a couple of days, it
might not be the weekend before I have time to dig through this.

Thanks,
Eric

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  62845@debbugs.gnu.org
> Date: Wed, 26 Apr 2023 20:08:53 -0700
> 
> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
> > commit 032969e8c65 "Don't have nntp-report signal an error"
> 
> Ooh, I knew this would end up being me. Give me a couple of days, it
> might not be the weekend before I have time to dig through this.

Eric,

Any progress?  I'd like to make another pretest of Emacs 29 soon, and
I'm waiting for this fix.  TIA.

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  62845@debbugs.gnu.org
>> Date: Wed, 26 Apr 2023 20:08:53 -0700
>> 
>> Andreas Schwab <schwab@linux-m68k.org> writes:
>> 
>> > commit 032969e8c65 "Don't have nntp-report signal an error"
>> 
>> Ooh, I knew this would end up being me. Give me a couple of days, it
>> might not be the weekend before I have time to dig through this.
>
> Eric,
>
> Any progress?  I'd like to make another pretest of Emacs 29 soon, and
> I'm waiting for this fix.  TIA.

Sorry this has been slow!

First of all, thanks very much to Andreas for pinpointing the cause of
this. I'd seen this before and looked for the cause a couple of times,
but obviously didn't look hard enough.

So in `nntp-finish-retrieve-group-infos' we have code that 
does this (with all the extraneous bits left out):

(with-current-buffer nntp-server-buffer
  (while <loop>
    <timer could kill nntp-server-buffer somewhere in here>
    (nntp-accept-response))
  (nnheader-strip-cr))

Where `nntp-accept-response' calls `nntp-report' if it can't find its
process, which it can't if `nntp-server-buffer` has been killed in the
interim.

`nntp-report' used to raise an error in this case, so it didn't matter
what happened afterwards. That error would derail the entire process of
Gnus checking for news/mail, so I took it out so that other servers
could continue doing their work even if this one had lost its
connection.

But then the current buffer of `with-current-buffer` is gone, which as
Andreas notes dumps us in the most-recently-used buffer, which could be
anything. The `nnheader-strip-cr' acts on that buffer, and terrible
things result.

Other code in this library checks if the timer has killed the process
buffer in the meantime. There's probably a safe solution in here
somewhere, but if you're looking for a reliable regression fix to
include in Emacs 29, it's probably best just to revert 032969e8c65. That
behavior is annoying, but at least not buggy.

WDYT?

Eric

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>>> Cc: Eli Zaretskii <eliz@gnu.org>,  62845@debbugs.gnu.org
>>> Date: Wed, 26 Apr 2023 20:08:53 -0700
>>> 
>>> Andreas Schwab <schwab@linux-m68k.org> writes:
>>> 
>>> > commit 032969e8c65 "Don't have nntp-report signal an error"
>>> 
>>> Ooh, I knew this would end up being me. Give me a couple of days, it
>>> might not be the weekend before I have time to dig through this.
>>
>> Eric,
>>
>> Any progress?  I'd like to make another pretest of Emacs 29 soon, and
>> I'm waiting for this fix.  TIA.

[...]

> Other code in this library checks if the timer has killed the process
> buffer in the meantime. There's probably a safe solution in here
> somewhere, but if you're looking for a reliable regression fix to
> include in Emacs 29, it's probably best just to revert 032969e8c65. That
> behavior is annoying, but at least not buggy.

Looking more closely at this, there's already a mechanism for throwing
out of the `nntp-with-open-group' wrapper: if `nntp--report-1' is t,
then `nntp-report' should throw the appropriate symbol and we'd get the
desired effect of canceling this server connection, without raising a
top-level error.

`nntp--report-1' should be non-nil in the case, I'll try to figure out
why it isn't working.

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>>>> Cc: Eli Zaretskii <eliz@gnu.org>,  62845@debbugs.gnu.org
>>>> Date: Wed, 26 Apr 2023 20:08:53 -0700
>>>> 
>>>> Andreas Schwab <schwab@linux-m68k.org> writes:
>>>> 
>>>> > commit 032969e8c65 "Don't have nntp-report signal an error"
>>>> 
>>>> Ooh, I knew this would end up being me. Give me a couple of days, it
>>>> might not be the weekend before I have time to dig through this.
>>>
>>> Eric,
>>>
>>> Any progress?  I'd like to make another pretest of Emacs 29 soon, and
>>> I'm waiting for this fix.  TIA.
>
> [...]
>
>> Other code in this library checks if the timer has killed the process
>> buffer in the meantime. There's probably a safe solution in here
>> somewhere, but if you're looking for a reliable regression fix to
>> include in Emacs 29, it's probably best just to revert 032969e8c65. That
>> behavior is annoying, but at least not buggy.
>
> Looking more closely at this, there's already a mechanism for throwing
> out of the `nntp-with-open-group' wrapper: if `nntp--report-1' is t,
> then `nntp-report' should throw the appropriate symbol and we'd get the
> desired effect of canceling this server connection, without raising a
> top-level error.
>
> `nntp--report-1' should be non-nil in the case, I'll try to figure out
> why it isn't working.

The answer is, that mechanism is designed to work only once. If the
connection is dead or times out, it catches that condition once and
tries to re-connect, and won't catch it a second time. So we'd have to
be hitting the timeout twice in a row to see this (which definitely
happens). I still think it's best just to revert 032969e8c65 on Emacs 29.

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Andreas Schwab]

On Mai 05 2023, Eric Abrahamsen wrote:

> The answer is, that mechanism is designed to work only once. If the
> connection is dead or times out, it catches that condition once and
> tries to re-connect, and won't catch it a second time. So we'd have to
> be hitting the timeout twice in a row to see this (which definitely
> happens). I still think it's best just to revert 032969e8c65 on Emacs 29.

When the timeout happens the server buffer is killed.  Reopening the
connection creates a new buffer, but the current buffer remains dead.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

On 05/06/23 08:13 AM, Andreas Schwab wrote:
> On Mai 05 2023, Eric Abrahamsen wrote:
>
>> The answer is, that mechanism is designed to work only once. If the
>> connection is dead or times out, it catches that condition once and
>> tries to re-connect, and won't catch it a second time. So we'd have to
>> be hitting the timeout twice in a row to see this (which definitely
>> happens). I still think it's best just to revert 032969e8c65 on Emacs 29.
>
> When the timeout happens the server buffer is killed.  Reopening the
> connection creates a new buffer, but the current buffer remains dead.

But doesn't the call to `nntp-possibly-change-group' at nntp.el:600
re-create the connection, including re-initializing the
nntp-server-buffer, so that when the guts of
`nntp-finish-retrieve-group-infos' are run for the second time, the new
server buffer is found?

I'm having a hell of a time reasoning about this whole flow.

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Andreas Schwab]

On Mai 06 2023, Eric Abrahamsen wrote:

> But doesn't the call to `nntp-possibly-change-group' at nntp.el:600
> re-create the connection, including re-initializing the
> nntp-server-buffer, so that when the guts of
> `nntp-finish-retrieve-group-infos' are run for the second time, the new
> server buffer is found?

You cannot recreate a buffer once it is killed.  You can create a new
buffer, but it will always be a different buffer.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

On 05/06/23 19:43 PM, Andreas Schwab wrote:
> On Mai 06 2023, Eric Abrahamsen wrote:
>
>> But doesn't the call to `nntp-possibly-change-group' at nntp.el:600
>> re-create the connection, including re-initializing the
>> nntp-server-buffer, so that when the guts of
>> `nntp-finish-retrieve-group-infos' are run for the second time, the new
>> server buffer is found?
>
> You cannot recreate a buffer once it is killed.  You can create a new
> buffer, but it will always be a different buffer.

But `nntp-possibly-change-group' re-initializes `nntp-server-buffer' to
a new buffer, and the `nntp-find-connection-buffer' inside
`nntp-retrieve-group-data-early' uses the value of `nntp-server-buffer'
to find its process. My reading is that each time the with-open-group
function runs, its `bodyfun' lambda should get a new opportunity to find
a live `nntp-server-buffer'.

Plus, if the buffer were already dead the second time through,
`nntp-retrieve-group-data-early' has sufficient guards to simply bail
before doing anything.

I'm not trying to be stubborn here, I assume your analysis is
essentially right, I'm just trying to make sure I actually understand
what's happening in the code.

Even if it does require two timeouts in a row, I get that plenty often
with a `nntp-connection-timeout' value of 6.

Eric

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Andreas Schwab]

On Mai 06 2023, Eric Abrahamsen wrote:

> But `nntp-possibly-change-group' re-initializes `nntp-server-buffer' to
> a new buffer, and the `nntp-find-connection-buffer' inside
> `nntp-retrieve-group-data-early' uses the value of `nntp-server-buffer'
> to find its process. My reading is that each time the with-open-group
> function runs, its `bodyfun' lambda should get a new opportunity to find
> a live `nntp-server-buffer'.

But that only happens when nntp-report-error is called.  The timer has
already killed the process buffer long before, and in the mean time the
function happily butchers around in a random buffer.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  62845@debbugs.gnu.org
> Date: Sat, 06 May 2023 22:52:17 -0700
> 
> 
> On 05/06/23 19:43 PM, Andreas Schwab wrote:
> > On Mai 06 2023, Eric Abrahamsen wrote:
> >
> > You cannot recreate a buffer once it is killed.  You can create a new
> > buffer, but it will always be a different buffer.
> 
> But `nntp-possibly-change-group' re-initializes `nntp-server-buffer' to
> a new buffer, and the `nntp-find-connection-buffer' inside
> `nntp-retrieve-group-data-early' uses the value of `nntp-server-buffer'
> to find its process. My reading is that each time the with-open-group
> function runs, its `bodyfun' lambda should get a new opportunity to find
> a live `nntp-server-buffer'.
> 
> Plus, if the buffer were already dead the second time through,
> `nntp-retrieve-group-data-early' has sufficient guards to simply bail
> before doing anything.
> 
> I'm not trying to be stubborn here, I assume your analysis is
> essentially right, I'm just trying to make sure I actually understand
> what's happening in the code.
> 
> Even if it does require two timeouts in a row, I get that plenty often
> with a `nntp-connection-timeout' value of 6.

Eric, any progress here?

If no better ideas are ready to be implemented, I think we should
revert that commit on emacs-29 and leave it on master, where work on
fixing this fallout should continue.  OK?

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

On 05/10/23 18:53 PM, Eli Zaretskii wrote:
>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  62845@debbugs.gnu.org
>> Date: Sat, 06 May 2023 22:52:17 -0700
>> 
>> 
>> On 05/06/23 19:43 PM, Andreas Schwab wrote:
>> > On Mai 06 2023, Eric Abrahamsen wrote:
>> >
>> > You cannot recreate a buffer once it is killed.  You can create a new
>> > buffer, but it will always be a different buffer.
>> 
>> But `nntp-possibly-change-group' re-initializes `nntp-server-buffer' to
>> a new buffer, and the `nntp-find-connection-buffer' inside
>> `nntp-retrieve-group-data-early' uses the value of `nntp-server-buffer'
>> to find its process. My reading is that each time the with-open-group
>> function runs, its `bodyfun' lambda should get a new opportunity to find
>> a live `nntp-server-buffer'.
>> 
>> Plus, if the buffer were already dead the second time through,
>> `nntp-retrieve-group-data-early' has sufficient guards to simply bail
>> before doing anything.
>> 
>> I'm not trying to be stubborn here, I assume your analysis is
>> essentially right, I'm just trying to make sure I actually understand
>> what's happening in the code.
>> 
>> Even if it does require two timeouts in a row, I get that plenty often
>> with a `nntp-connection-timeout' value of 6.
>
> Eric, any progress here?
>
> If no better ideas are ready to be implemented, I think we should
> revert that commit on emacs-29 and leave it on master, where work on
> fixing this fallout should continue.  OK?

Yeah, I haven't made much progress in the time I've had to look at this
(and I still don't completely understand the current error!).

I think the commit should be reverted on both emacs-29 and master. It
was there to fix an inconvenience (an nntp failure would halt a Gnus
refresh at the top level), but it caused an actual bug (Gnus messes with
the contents of random buffers). Better to go back to the inconvenience
until a fuller solution is found.

Thanks,
Eric

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Cc: schwab@linux-m68k.org,  62845@debbugs.gnu.org
> Date: Wed, 10 May 2023 11:21:42 -0700
> 
> 
> > Eric, any progress here?
> >
> > If no better ideas are ready to be implemented, I think we should
> > revert that commit on emacs-29 and leave it on master, where work on
> > fixing this fallout should continue.  OK?
> 
> Yeah, I haven't made much progress in the time I've had to look at this
> (and I still don't completely understand the current error!).
> 
> I think the commit should be reverted on both emacs-29 and master. It
> was there to fix an inconvenience (an nntp failure would halt a Gnus
> refresh at the top level), but it caused an actual bug (Gnus messes with
> the contents of random buffers). Better to go back to the inconvenience
> until a fuller solution is found.

Thanks, I've now reverted commit 032969e8c65 on the emacs-29 branch;
soon to be merged to master.

Should I now close this bug, or do you want it to stay open?

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

On 05/11/23 13:01 PM, Eli Zaretskii wrote:
>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Cc: schwab@linux-m68k.org,  62845@debbugs.gnu.org
>> Date: Wed, 10 May 2023 11:21:42 -0700
>> 
>> 
>> > Eric, any progress here?
>> >
>> > If no better ideas are ready to be implemented, I think we should
>> > revert that commit on emacs-29 and leave it on master, where work on
>> > fixing this fallout should continue.  OK?
>> 
>> Yeah, I haven't made much progress in the time I've had to look at this
>> (and I still don't completely understand the current error!).
>> 
>> I think the commit should be reverted on both emacs-29 and master. It
>> was there to fix an inconvenience (an nntp failure would halt a Gnus
>> refresh at the top level), but it caused an actual bug (Gnus messes with
>> the contents of random buffers). Better to go back to the inconvenience
>> until a fuller solution is found.
>
> Thanks, I've now reverted commit 032969e8c65 on the emacs-29 branch;
> soon to be merged to master.
>
> Should I now close this bug, or do you want it to stay open?

Please leave it open, thanks!

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Cc: schwab@linux-m68k.org,  62845@debbugs.gnu.org
> Date: Fri, 05 May 2023 17:03:35 -0700
> 
> `nntp-report' used to raise an error in this case, so it didn't matter
> what happened afterwards. That error would derail the entire process of
> Gnus checking for news/mail, so I took it out so that other servers
> could continue doing their work even if this one had lost its
> connection.
> 
> But then the current buffer of `with-current-buffer` is gone, which as
> Andreas notes dumps us in the most-recently-used buffer, which could be
> anything. The `nnheader-strip-cr' acts on that buffer, and terrible
> things result.
> 
> Other code in this library checks if the timer has killed the process
> buffer in the meantime. There's probably a safe solution in here
> somewhere, but if you're looking for a reliable regression fix to
> include in Emacs 29, it's probably best just to revert 032969e8c65. That
> behavior is annoying, but at least not buggy.
> 
> WDYT?

Fine with me, but what is the plan for master?  If you can show the
proposed solution for master, we could then try to figure out if it is
safe enough for emacs-29 as well.

But if it will take a significant time to come up with such a solution
for master, then let's revert on emacs-29 for now.

Thanks.

bug#62845: 29.0.90; nntp-with-open-group-function kills current buffer on timeout

[Eric Abrahamsen]

On 05/06/23 09:31 AM, Eli Zaretskii wrote:
>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Cc: schwab@linux-m68k.org,  62845@debbugs.gnu.org
>> Date: Fri, 05 May 2023 17:03:35 -0700
>> 
>> `nntp-report' used to raise an error in this case, so it didn't matter
>> what happened afterwards. That error would derail the entire process of
>> Gnus checking for news/mail, so I took it out so that other servers
>> could continue doing their work even if this one had lost its
>> connection.
>> 
>> But then the current buffer of `with-current-buffer` is gone, which as
>> Andreas notes dumps us in the most-recently-used buffer, which could be
>> anything. The `nnheader-strip-cr' acts on that buffer, and terrible
>> things result.
>> 
>> Other code in this library checks if the timer has killed the process
>> buffer in the meantime. There's probably a safe solution in here
>> somewhere, but if you're looking for a reliable regression fix to
>> include in Emacs 29, it's probably best just to revert 032969e8c65. That
>> behavior is annoying, but at least not buggy.
>> 
>> WDYT?
>
> Fine with me, but what is the plan for master?  If you can show the
> proposed solution for master, we could then try to figure out if it is
> safe enough for emacs-29 as well.
>
> But if it will take a significant time to come up with such a solution
> for master, then let's revert on emacs-29 for now.

I've had a patch sitting around since forever that defines a handful of
Gnus-specific errors and uses them for explicit flow control, preventing
errors with individual servers from interrupting Gnus' top-level usage.
Personally I think that's the right way to go, but it seems like too
much for a emacs 29 fix. I will look the patch over this afternoon and
see how close I got.