On Wed, Oct 19, 2022 at 04:20:22PM -0700, Kevin J. McCarthy wrote: > 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.
Whatever will cause less churn / work for you is fine with me. I wouldn't mind throwing my name on a mutt commit, but if it will cause less back and forth for you to just write it how you'd like that's fine as well. If you do, please feel free to ping me for a review. > > 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. Understood, let's leave this alone, then. > > > 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. :) Ah, ok, good to know. > > > 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. Ok, I see. So the intention of $timeout is / was really to avoid the user experiencing latency when they eventually do provide input, due to some background task having kicked in. > 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. Agreed, another variable sounds like it would have been more appropriate to prevent this. > 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. Understood, being a Linux kernel contributor I'm used to this constraint. We must reward our loyal users with the expectation that we won't pull the rug out from under them, even when "fixing" a bug to match what's documented :-) It is what it is, and honestly, fixing the documentation to describe the behavior and its implications is probably sufficient given the available workaround of setting a large timeout anyways. Perhaps adding a comment in the code is warranted as well. > 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. If you feel this is the best / lightest touch, I'm happy to send out a patch that does it. For my own clarification though, what would we gain from doing this if keepalive is also sending the noops? Wouldn't this just result in the same behavior, expect that we may even be sending multiple, redundant NOOPs? > Perhaps just setting $timeout to 32000 would accomplish what you want > better? Yes, this was the workaround I did for the mutt version I'm using which is packaged with my distro, after realizing the latency issues were due to Timeout <= 0 not being applied as documented. It's fine with me to use this as the workaround if the preference is to not disturb the codebase, especially knowing that I can rely on that behavior never changing. > > 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). Yes, I think another variable would / should have been used for this. I would propose my sending a patch out that adds such a variable, but given that mutt is strictly in bux-fix-only mode, I can just stick with the $timeout = <large> workaround you suggested above, and one of us can send a follow-on patch which instead properly documents the behavior of $timeout. Thanks, David
