[leafnode-list] Re: Strange problem with 2.0.0.alpha20050412a

[Matthias Andree]

"John Carlyle-Clarke" <john.cc at europlacer.co.uk> writes:

> I can quite understand that Leafnode cannot work around a broken 
> server - in the sense of manage to retrieve news from it - but is it 
> expected that the fetch would hang, and that it would fail to retrieve 
> news from any servers, which was what seemed to be happening?

Turns out that the code needs some restructuring before we can get to a
real solution. This patch should make fetchnews give up quicker and
proceed to the next server. Applies on top of 20050422a, or you can pull
from DARCS. <http://home.pages.de/~mandree/leafnode/beta/darcs/>

diff -rN -u old-leafnode-2/NEWS new-leafnode-2/NEWS
--- old-leafnode-2/NEWS	2005-04-22 09:40:34.000000000 +0200
+++ new-leafnode-2/NEWS	2005-05-10 23:27:14.000000000 +0200
@@ -2,6 +2,11 @@
 * Valgrind-2.1.X will interfere with writeactive, break fchmod. Use 2.0.
 * The code sometimes uses timeout_client where it should use a server timeout.
 |
+Changes since 20050422a:
+- Stability: If getting a non-text reply in response to ARTICLE, leave
+  current server alone on the assumption we're out of synch and proceed
+  to next.
+
 20050422a: Changes since 20050412a:
 - Bugfix: texpire can crash for some spool states (on incomplete
   groupinfo), reported by John Carlyle-Clarke.
diff -rN -u old-leafnode-2/fetchnews.c new-leafnode-2/fetchnews.c
--- old-leafnode-2/fetchnews.c	2005-03-24 13:24:19.000000000 +0100
+++ new-leafnode-2/fetchnews.c	2005-05-11 00:05:57.000000000 +0200
@@ -380,6 +380,11 @@
 
 /**
  * Get a single article and apply filters.
+ *
+ * XXX FIXME: do we want a "reconnect to current server and continue"
+ * state when we're out of synch? We'd need to track if we're making
+ * progress so we don't loop on the same b0rked article for instance.
+ *
  * \return
  *  - -2: server disconnected or OS error in store, abort the fetch
  *  - -1: did not get a 22X reply, continue the fetch
@@ -392,7 +397,7 @@
     int delayflg)
 {
     char *l;
-    int reply = 0;
+    int reply = 0, argcount;
 
     l = mgetaline(nntpin);
     if (!l) {
@@ -401,10 +406,11 @@
 	return -2;
     }
 
-    if ((sscanf(l, "%3d %lu", &reply, artno) != 2) || (reply / 10 != 22)) {
+    if (((argcount = sscanf(l, "%3d %lu", &reply, artno)) < 2)
+	    || (reply / 10 != 22)) {
 	ln_log(LNLOG_SNOTICE, LNLOG_CARTICLE,
-	       "Wrong reply to ARTICLE command: %s", l);
-	if (reply / 100 == 5)
+	       "Wrong reply to ARTICLE command: \"%s\"", l);
+	if (argcount < 2 || reply / 100 == 5)
 	    return -2;		/* fatal error */
 	return -1;
     }
@@ -712,7 +718,7 @@
     if (!parsegroupreply((const char **)&t, &u, &h, &window, last)) {
 	ln_log(LNLOG_SERR, LNLOG_CGROUP, "%s: cannot parse reply to GROUP %s: \"%s\"",
 		cursrv->name, g->name, l);
-	return 0;
+	return -2;
     }
 
     if (u != 211) {
@@ -1115,6 +1121,7 @@
 /**
  * fetch all articles for that group.
  * \return
+ * - -2 to abort fetch from current server
  * - 0 for error or if group is unavailable
  * - otherwise last article number in that group
  */
@@ -1176,6 +1183,8 @@
 	return first;
     case -1:
 	return 0;
+    case -2:
+	return -2;
     default:
 	break;
     }
@@ -1585,6 +1594,7 @@
     RBLIST *r = NULL;			/* interesting groups pointer */
     struct rbtree *upstream;		/* upstream water marks */
     int rc = 0;				/* return value */
+    int fault = 0;			/* if we skip the rest of the groups */
 
     /* read info */
     oldfile = server_info(spooldir, server, port, "");
@@ -1649,16 +1659,18 @@
 	    /* we still want this group */
 
 	    g = findgroup(ng, active, -1);
-            if (!g) {
-                if (!forceactive && (debugmode & DEBUG_ACTIVE))
-                    ln_log(LNLOG_SINFO, LNLOG_CGROUP,
-                            "%s not found in groupinfo file", ng);
-            }
+	    if (!g) {
+		if (!forceactive && (debugmode & DEBUG_ACTIVE))
+		    ln_log(LNLOG_SINFO, LNLOG_CGROUP,
+			    "%s not found in groupinfo file", ng);
+	    }
 
-	    newserver = getgroup(cursrv, g, from);
+	    if (fault == 0)
+		newserver = getgroup(cursrv, g, from);
+	    else
+		newserver = 0;
 	    if (newserver == (unsigned long)-2) { /* fatal */
-		/* FIXME: write back all other upstream entries, return */
-		goto out;
+		fault = 1;
 	    }
 	    /* write back as good info as we have, drop if no real info */
 	    if (newserver != 0) {
@@ -1694,7 +1706,7 @@
 
     free(newfile);
     free(oldfile);
-    return rc;
+    return fault ? 0 : rc;
 }
 
 /**



-- 
Matthias Andree