On Wed, Oct 19, 2022 at 02:16:12PM -0500, David Vernet wrote:
On Wed, Oct 19, 2022 at 10:47:26AM -0700, Kevin J. McCarthy wrote: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.
That's fine, or I'll be happy to touch it up myself after this is resolved.
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 don't think it will change within the loop. But right now, Mutt is in bug-fix mode, so I'd like to keep fixes targeted towards actual bug fixes. It looks redundant to me too, but unless there is a problem occurring I'd prefer to just leave as-is.
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).
Err sorry... I did forget that the keepalive was sending NOOPs too, and the main loop mx_check_mailbox() will pick up the new mail when REOPEN is set. So I retract the above. :)
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.
I think this expectation is outside of the scope of $timeout too. $timeout is really about how long to wait at the menu for input before giving up and "doing things" like checking mail or buffy. But (as documented) it doesn't deal with behavior once outside the menu prompt. I don't think setting it to 0 to avoid pinging the IMAP server, even when *outside* the prompt, is expected by users at this point.
I'm guessing Timeout was added to imap_check_mailbox() to avoid sending NOOP's constantly when scrolling or such, as a kind of hack. This does behave badly when $timeout is 0 (or negative). Perhaps there should have been another variable added originally.
It also may have been more correct to set i to INT_MAX in km_dokey() for the 0/negative case. We could do that, but I'd argue we have the reverse problem now: the behavior has been as-is for 20+ years. Even though the doc's say it doesn't timeout, it does, and it does end up checking current mailbox/buffy about once a minute.
I still think it would be better to just change the $timeout doc to say a 0/negative value will default to 60, and just change imap_check_mailbox() to do the same, to make minimal behavior changes right now.
Perhaps just setting $timeout to 32000 would accomplish what you want better?
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 your understanding of keepalive is correct (although, again, the $timeout documentation needs to be fixed here, since km_dokey() is checking $imap_keepalive). However, again, I don't agree this is to enable the user to avoid pinging the IMAP server if they don't want to (by setting $timeout to 0).
I'm out of time today, but I'll try to look through this part of the code again tomorrow to see if I'm missing or misunderstanding things. (It's been a while since I've gone through this area).
-- Kevin J. McCarthy GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
