HTHost: Pipeline recovery inherently broken [patch]

Hi,

in my continuing attempts to get libwww to resume FTP and HTTP downloads 
properly, I've come across another problem - libwww's behaviour if the 
connection to a HTTP 1.1 pipelining server suddenly drops.

If the connection drops with FTP or HTTP 1.0 or non-pipelined HTTP 1.1, 
then the application gets back an error, which is the right thing.

If the connection drops with HTTP 1.1 and a pipeline in place, libwww will
"recover the pipeline" _exactly_once_ (#define MAX_HOST_RECOVER 1), by
re-connecting to the server and re-adding all requests which were
registered just before the connection dropped.

This is stupid, for a number of reasons:

  - It is not consistent with the FTP/HTTP1.0/HTTP1.1-non-pipelined case. 
    Chances are the application will not expect it.
    
  - Rather than making life easier for the app, it makes it more difficult, 
    because the app *still* has to cater for the case that the connection
    drops again after the retry, in which case it needs to re-schedule the
    other requests of that pipeline etc.
  
  - There's no way to customize the number of retries. Why exactly once? 
    There's also no way to prevent libwww from doing this.
  
  - Most importantly: The request that was active at the time the 
    connection dropped is *restarted* (not resumed at the right byte
    offset!), and its data is sent to the output stream without further 
    notice, i.e. the output stream will think that nothing has happened and 
    that the bytes it has just been given are for one continuous download.
    
The attached patch simply disables the "pipeline recovery" feature. A 
cleaner way would be to remove the recovery code altogether, since IMHO 
libwww is just trying to be too clever here - it should leave the error 
situation (dropped connection) for the application to deal with.

[The alternative approach to fixing this would be still *more*
intelligence: Make it resume the hitherto active request from the right
byte offset. But I don't like this, because 1) the app should be notified
of such an error, 2) the app will need to have code dealing with dropped
connections anyway, so why reproduce it in libwww, 3) libwww has no way of
knowing whether resuming is the right action; for example, the app may
actually want to resume the download using a different, hopefully better
mirror.]

Apart from setting MAX_HOST_RECOVER=0, the patch also fixes an off-by-one 
error (>= instead of >), and it prevents that pipelining is switched off 
altogether with MAX_HOST_RECOVER==0 - with the current code, pipelining is 
switched off for the MAX_HOST_RECOVERth retry.

Cheers,

  Richard

-- 
  __   _
  |_) /|  Richard Atterer     |  CS student at the Technische  |  GnuPG key:
  | \/¯|  http://atterer.net  |  Universität München, Germany  |  0x888354F7
  ¯ '` ¯

--- HTHost.c.orig	2003-02-17 23:43:12.000000000 +0100
+++ HTHost.c	2003-02-18 00:58:12.000000000 +0100
@@ -31,7 +31,7 @@
 #define TCP_IDLE_ACTIVE     60000L /* Active TTL in ms on an idle connection */
 
 #define MAX_PIPES		50   /* maximum number of pipelined requests */
-#define MAX_HOST_RECOVER	1	      /* Max number of auto recovery */
+#define MAX_HOST_RECOVER	0	      /* Max number of auto recovery */
 #define DEFAULT_DELAY		30	  /* Default write flush delay in ms */
 
 struct _HTInputStream {
@@ -824,7 +824,7 @@
 	/*
 	**  First check that we haven't already recovered more than we want
 	*/
-	if (host->recovered > MAX_HOST_RECOVER) {
+	if (host->recovered >= MAX_HOST_RECOVER) {
 	    HTTRACE(CORE_TRACE, "Host recover %p already %d times - not doing it anymore\n" _ host _ host->recovered);
 	    return NO;
 	}
@@ -958,7 +958,7 @@
     case HT_TP_SINGLE:
 	return count <= 0;
     case HT_TP_PIPELINE:
-	return (host->recovered < MAX_HOST_RECOVER) ?
+	return (host->recovered < MAX_HOST_RECOVER || MAX_HOST_RECOVER==0) ?
 	    (count < MaxPipelinedRequests) : (count <= 0);
     case HT_TP_INTERLEAVE:
 	return YES;

Received on Wednesday, 19 February 2003 16:54:06 UTC