Re: Patch to work around flaky pop3 server behaviour

[Pawel Salek <pawsa theochem kth se>]

On 2001.06.09 00:42 Thomas Fitzsimmons wrote:
> This patch makes pop3 mail retrieval more robust.
> 
> It uses a non-blocking read function when retrieving characters from the
> server, so that the mail retrieval thread doesn't hang when the server
> fails to send an expected response.

This patch looks, hm, strange, to me. In particular, i is a local variable,
and there is no loop to iterate over retry attepmts (the other option is
that I am blind, or very, very tired).

I would rather commit another, attached patch. It is simpler and really
works. The potential disadvantage is that it uses POSIX signals (OTOH, it
implements commonly used technique so it should be OK).

Comments?

/Pawel

> + static int
> + safe_read_char(int fd, char *ch)
> + {
> +     int i = 0;
> +     int retval = 0;
> + 
> +     /* make read non-blocking */
> +     fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK);
> +     retval = read (fd, ch, 1);
> +     /* make read blocking again */
> +     fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) ^ O_NONBLOCK);
> +     
> +     if (retval == -1) {
> +         
> +         if (errno == EAGAIN) {
> +             /* pause one second before trying again */
> +             sleep(1);
> +         } else return -1;
> +         
> +         if(i < NUMBER_OF_RETRIES) {
> +             fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK);
> +             retval = read (fd, ch, 1);
> +             fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) ^ O_NONBLOCK);
> +         } else return -1;
> +         
> +         i++;
> +     } else return retval;
> +     
> +     /* make gcc happy */
> +     return retval;
> + }
> + 
>   static int getLine (int fd, char *s, int len)
>   {
>       char ch;
>       int bytes = 0;
>       
> !     while (safe_read_char (fd, &ch) > 0) {
>   	*s++ = ch;
>   	bytes++;
>   	if (ch == '\n') {
> 
-- 
-----
Pawel Salek, pawsa@theochem.kth.se
Index: libbalsa/pop3.c
===================================================================
RCS file: /cvs/gnome/balsa/libbalsa/pop3.c,v
retrieving revision 1.3
diff -u -r1.3 pop3.c
--- libbalsa/pop3.c	2000/11/16 19:43:13	1.3
+++ libbalsa/pop3.c	2001/06/09 10:11:49
@@ -32,6 +32,8 @@
 #include <netdb.h>
 #include <string.h>
 
+#include <errno.h>
+#include <signal.h>
 #include <unistd.h>
 #include <stdio.h>
 #include <gnome.h>
@@ -61,27 +63,52 @@
     return _(errmsgs[status]);
 }
 
+
+#ifdef BALSA_TEST_POP3
+#define DM( fmt, args... ) G_STMT_START { fprintf( stderr, "POP3 D: " ); fprintf( stderr, fmt, ##args ); fprintf( stderr, "\n" ); } G_STMT_END
+#else
+#define DM( fmt, args... ) 
+#endif
+
+/* safe_read_char:
+        Reads a character with a specified POP_TIMEOUT value.
+*/
+static void 
+null_signal_alarm_handler(int i) 
+{
+    g_warning("Timeout occured in POP input reading.\n");
+}
+static int
+safe_read_char(int fd, char *ch)
+{
+    static const int POP_TIMEOUT=20;
+    int retval;
+
+    if( (retval=alarm(POP_TIMEOUT)) != 0) /* timeout value: 30 seconds */
+	g_warning("Cancelled previously scheduled alarm (%d secs).",
+		  retval);
+    retval = read (fd, ch, 1);
+    alarm(0);
+    return retval;
+}
+
 static int getLine (int fd, char *s, int len)
 {
     char ch;
     int bytes = 0;
     
-    while (read (fd, &ch, 1) > 0) {
+    while (safe_read_char (fd, &ch) > 0) {
 	*s++ = ch;
 	bytes++;
 	if (ch == '\n') {
 	    *s = 0;
-#ifdef BALSA_TEST_POP3
-	    fprintf( stderr, "POP3 L: \"%s\"\n", (char *) (s - bytes) );
-#endif
+	    DM("POP3 L: \"%s\"\n", (char *) (s - bytes) );
 	    return bytes;
 	}
       /* make sure not to overwrite the buffer */
 	if (bytes == len - 1) {
 	    *s = 0;
-#ifdef BALSA_TEST_POP3
-	    fprintf( stderr, "POP3 L: \"%s\"\n", (char *) (s - bytes) );
-#endif
+	    DM("POP3 L: \"%s\"\n", (char *) (s - bytes) );
 	    return bytes;
 	}
     }
@@ -89,12 +116,6 @@
     return -1;
 }
 
-#ifdef BALSA_TEST_POP3
-#define DM( fmt, args... ) G_STMT_START { fprintf( stderr, "POP3 D: " ); fprintf( stderr, fmt, ##args ); fprintf( stderr, "\n" ); } G_STMT_END
-#else
-#define DM( fmt, args... ) 
-#endif
-
 /* getApopStamp:
    Get the Server Timestamp for APOP authentication -kabir 
    return TRUE on success.
@@ -317,6 +338,8 @@
 fetch_single_msg(int s, FILE *msg, int msgno, int first_msg, int msgs, 
 		 int *num_bytes, int tot_bytes, ProgressCallback prog_cb) {
     char buffer[2048];
+    static const int GUI_PROGRESS_STEP = 2048;
+    int last_update_size = 0;
 #ifdef BALSA_USE_THREADS
     char threadbuf[160];
     
@@ -333,8 +356,12 @@
 	if ((chunk = getLine (s, buffer, sizeof (buffer))) == -1) 
 	    return POP_CONN_ERR;
 #ifdef BALSA_USE_THREADS
-	sprintf(threadbuf,_("Received %d bytes of %d"), *num_bytes, tot_bytes);
-	prog_cb (threadbuf, *num_bytes, tot_bytes);
+	if(last_update_size<*num_bytes) {
+	    sprintf(threadbuf,_("Received %d bytes of %d"), 
+		    *num_bytes, tot_bytes);
+	    prog_cb (threadbuf, *num_bytes, tot_bytes);
+	    last_update_size += GUI_PROGRESS_STEP;
+	}
 #endif
 	/* check to see if we got a full line */
 	if (buffer[chunk-2] == '\r' && buffer[chunk-1] == '\n') {
@@ -481,7 +508,14 @@
     return err;
 }
 
-/* ------------------------------------------------------------------- */
+/* fetch_pop_mail:
+   loads the mail down from a POP server.
+   Can handle connection timeouts by using alarm signal handler.
+   Before each read, a alarm() is set. The read is interrupted if it blocked
+   too long.
+   The signal handler is iset in fetch_pop_mail and the reading itself is
+   done by safe_read_char()
+*/
 typedef PopStatus
 (*ProcessCallback)(int s, gint first_msg, gint msgs, gint tot_bytes,
 		   gboolean delete_on_server, const gchar * procmail_path,
@@ -498,10 +532,17 @@
     char uid[80];
     int s, msgs, bytes, first_msg;
     PopStatus status = POP_OK;
+    struct sigaction newsig, oldsig;
 
     g_return_val_if_fail(pop_host, POP_HOST_NOT_FOUND);
     g_return_val_if_fail(pop_user, POP_AUTH_FAILED);
 
+
+    newsig.sa_handler = null_signal_alarm_handler;
+    newsig.sa_flags   = 0;
+    sigemptyset(&newsig.sa_mask);
+    sigaction(SIGALRM, &newsig, &oldsig);
+
     DM("Begin connection");
     status = pop_connect(&s, pop_host, pop_port);
     DM("Connected; hello");
@@ -528,6 +569,7 @@
     }
     close (s);
 
+    sigaction(SIGALRM, &oldsig, NULL);
     return status;
 }