On Wed, Oct 19, 2022 at 10:47:26AM -0700, Kevin J. McCarthy wrote:
> Hi David,
>
> Thanks for the patch.
>
> On Wed, Oct 19, 2022 at 12:01:36PM -0500, David Vernet wrote:
> > In imap_check_mailbox(), there is an if clause where we potentially poll
> > IMAP with a NOOP command. This can be invoked on many ops, including if
> > we e.g. move to the next message on the index. According to
> > http://www.mutt.org/doc/manual/#timeout, the expected behavior of
> > Timeout is that a timeout value of <= 0 will cause Mutt to never time
> > out.
>
> It looks like "never timing out" used to be the case, but was slowly changed
> (e.g. commit 2fee61e1).
>
> Now, km_dokey() will change <=0 to 60, to make sure IMAP keepalive's run.
Ah, got it, thanks for pointing me to that code.
> The documentation should be fixed for that.
Ack, thanks for clarifying. I'm happy to send an update to the documentation as
part of this patch set as well if you'd like. Also, while perusing the code I
noticed that the first ImapKeepalive in km_dokey() is redundant as we
hav an if check for ImapKeepalive above it:
while (ImapKeepalive && ImapKeepalive < i)
So I can also include a small cleanup for that as well, assuming I'm not
wrong that it's constant / static for the runtime of the program.
> I agree imap_check_mailbox() should be fixed so it isn't blasting NOOPs all
> the time too. But the side effect of your patch would be to almost never
> send NOOPs, which would make Mutt very slow to notice new mail (unless
> $imap_idle is in use).
>
> What if it did the same thing as km_dokey(), changing <=0 to 60?
Hmm, my personal opinion is that silently changing the timeout to a
higher value may be unexpected / unwanted for users that really want to
avoid ever pinging the IMAP server so as to avoid latencies. Using
myself as an anecdotal esxample, I prefer to just manually refresh the
index / mailbox when I want mutt to look for new mail to ensure the most
predictable behavior / latencies. Timeout seems like the correct knob to
do that.
W.r.t. us bending the rules a bit for IMAP keepalive, IMO it feels like
IMAP keepalive is slightly different than checking for new mail in that
IMAP keepalive prevents annoying / long latencies that would result from
having to reauthenticate if the IMAP server dropped your connection due
to inactivity from _not_ polling for new mail. In other words, I think
it actually enables this specific use case of allowing the user to not
ping the IMAP server if they don't want to, by ensuring the connection
stays alive on the user's behalf according to the IMAP RFC.
I think that's evidenced by the fact that IMAP keepalive messages will
be sent even if they're shorter than the specified value for Timeout:
> i = Timeout > 0 ? Timeout : 60;
>#ifdef USE_IMAP
> /* keepalive may need to run more frequently than Timeout allows */
> if (ImapKeepalive)
> {
> if (ImapKeepalive >= i)
> imap_keepalive ();
> else
> while (ImapKeepalive && ImapKeepalive < i)
>
^^^ in this loop, we issue imap_keepalive() even if it's < Timeout
>
> {
> mutt_getch_timeout (ImapKeepalive * 1000);
> tmp = mutt_getch ();
> mutt_getch_timeout (-1);
> /* If a timeout was not received, or the window was resized, exit the
> * loop now. Otherwise, continue to loop until reaching a total of
> * $timeout seconds.
> */
>#ifdef USE_INOTIFY
> if (tmp.ch != -2 || SigWinch || MonitorFilesChanged)
>#else
> if (tmp.ch != -2 || SigWinch)
>#endif
> goto gotkey;
> i -= ImapKeepalive;
> imap_keepalive ();
> }
> }
>#endif
What do you think? It's certainly possible that I'm misunderstanding the
finer points of how some of this stuff works.
Thanks,
David
