[fetchmail-users] Re: [PATCH] Re: Bug in imap_idle() with mail servers not supporting "idle"

[Brendan L. <bre...@ai...>]

Your patch works too.  I cannot say the code is clearer than my fix, but 
you are no doubt a lot more familiar with this code than I am.  The key 
fix (beyond not waiting for a tagged response when no tagged command had 
been issued) in your fix is at line 595, where you do not attempt to 
enter the pseudo-idle wait if a RECENT message has been received 
(recentcount != 0).

Thanks,

Brendan

sh...@bo... wrote:

>Quoting from Brendan Lynch's mail on Thu, Mar 02, 2006:
>  
>
>>However I believe the servers is behaving correctly according to the 
>>IMAP spec: I have attached an explanation of the secondary problem I am 
>>seeing (extra 28 s wait) that I sent to Casper; let me know if you need 
>>more info.
>>    
>>
>
>Yes, I have now read RFC 3501 section 5.3. This behaviour is correct.
>
>  
>
>>The key point is that the imap_idle() code when 'faking' IDLE support 
>>calls imap_ok() (at line 637) without having issued any tagged command:
>>    
>>
>
>...
>
>  
>
>>Since there is no tagged command outstanding the server will not (and 
>>should not) ever send a tagged response.  If we set imap_ok to only 
>>return after a tagged response we are guaranteeing it will always only 
>>return after a timeout.
>>    
>>
>
>...
>
>  
>
>>This again first does a gen_transact() which results in sending "A0130 
>>NOOP" at 17:00:49.  A "RECENT" update is already on its way form the 
>>server, so we receive it before the "A0130 NOOP completed" at 17:00:49.  
>>In imap_ok() we first process the "* 1 RECENT" notification at line 117, 
>>setting recentcount, then (since we ARE waiting a tag) loop and process 
>>the NOOP ok.  we then return to gen_transact(), which in turn returns to 
>>imap_idle().
>>
>>Since setting "recentcount" does not also change the stage, we fail the 
>>test at line 633 of imap_idle() and continue to do an extra imap_ok() 
>>which sets a 28 second timer at 17:00:49 and does a recv() which times 
>>out at 17:01:17.  We then return from imap_idle() to imap_getrange() 
>>line 679.  At this point recentcount is set and we exit the "while 
>>(recentcount == 0 && do_idle)" loop.
>>
>>The wait between 17:00:49 and 17:01:17 was unnecessary, as already had 
>>our RECENT notification.  The second part of my fix causes imap_ok() to 
>>change the stage to STAGE_FETCH on receipt of a RECENT notification, 
>>just as it already did on receipt of a EXISTS notification. This causes 
>>the if condition "if (ok != 0 || stage != STAGE_IDLE)" to be met at line 
>>633 of imap_idle() so it immediately exits and does not do the extra 
>>unneeded imap_ok() call.
>>    
>>
>
>Yes, you have diagnosed the problem correctly and your patch is also
>correct. Based on your patch, I have prepared a new patch which does
>not modify imap_ok(), but cleans up the code in imap_idle(). Could you
>test this patch and see if this patch is acceptable to you? This patch
>is identical to the patch I had sent to Casper Gripenberg, except that
>comments relating to compliance with RFCs have been updated.
>
>  
>
>------------------------------------------------------------------------
>
>Index: fetchmail-6.3/imap.c
>===================================================================
>--- fetchmail-6.3/imap.c   (revision 4705)
>+++ fetchmail-6.3/imap.c   (working copy)
>@@ -621,7 +621,6 @@
> {
>     int ok;
> 
>-    stage = STAGE_IDLE;
>     saved_timeout = mytimeout;
> 
>     if (has_idle) {
>@@ -629,6 +628,7 @@
>    * at least every 28 minutes:
>    * (the server may have an inactivity timeout) */
>   mytimeout = 1680; /* 28 min */
>+  stage = STAGE_IDLE;
>   /* enter IDLE mode */
>   ok = gen_transact(sock, "IDLE");
> 
>@@ -637,37 +637,43 @@
>       SockWrite(sock, "DONE\r\n", 6);
>       if (outlevel >= O_MONITOR)
>       report(stdout, "IMAP> DONE\n");
>-  } else
>-      /* not idle timeout */
>-      return ok;
>+      /* reset stage and timeout here: we are not idling any more */
>+      mytimeout = saved_timeout;
>+      stage = STAGE_FETCH;
>+      /* get OK IDLE message */
>+      ok = imap_ok(sock, NULL);
>+  }
>     } else {  /* no idle support, fake it */
>-  /* when faking an idle, we can't assume the server will
>-   * send us the new messages out of the blue (RFC2060);
>-   * this timeout is potentially the delay before we notice
>-   * new mail (can be small since NOOP checking is cheap) */
>-  mytimeout = 28;
>+  /* Note: stage and timeout have not been changed here as NOOP
>+   * does not idle */
>   ok = gen_transact(sock, "NOOP");
>-  /* if there's an error (not likely) or we just found mail (stage 
>-   * has changed, timeout has also been restored), we're done */
>-  if (ok != 0 || stage != STAGE_IDLE)
>-      return(ok);
> 
>-  /* wait (briefly) for an unsolicited status update */
>-  ok = imap_ok(sock, NULL);
>-  /* again, this is new mail or an error */
>-  if (ok != PS_IDLETIMEOUT)
>-      return(ok);
>+  /* no error, but no new mail either */
>+  if (ok == PS_SUCCESS && recentcount == 0)
>+  {
>+      /* There are some servers who do send new mail
>+       * notification out of the blue. This is in compliance
>+       * with RFC 2060 section 5.3. Wait for that with a low
>+       * timeout */
>+      mytimeout = 28;
>+      stage = STAGE_IDLE;
>+      /* We are waiting for notification; no tag needed */
>+      tag[0] = '\0';
>+      /* wait (briefly) for an unsolicited status update */
>+      ok = imap_ok(sock, NULL);
>+      if (ok == PS_IDLETIMEOUT) {
>+      /* no notification came; ok */
>+      ok = PS_SUCCESS;
>+      }
>+  }
>     }
> 
>     /* restore normal timeout value */
>+    set_timeout(0);
>     mytimeout = saved_timeout;
>     stage = STAGE_FETCH;
> 
>-    /* get OK IDLE message */
>-    if (has_idle)
>-        return imap_ok(sock, NULL);
>-
>-    return PS_SUCCESS;
>+    return(ok);
> }
> 
> static int imap_getrange(int sock, 
>  
>