Sebastien Willemijns reported bug #1603712

(http://curl.haxx.se/bug/view.cgi?id=1603712) which is about connections
getting cut off prematurely when --limit-rate is used. While I found no such
problems in my tests nor in my reading of the code, I found that the
--limit-rate code was severly flawed (since it was moved into the lib, since
7.15.5) when used with the easy interface and it didn't work as documented so
I reworked it somewhat and now it works for my tests.

diff --git a/CHANGES b/CHANGES
index 60828fd3dd8613fa315e3c05fcbd8ec65bfa9bdf..4d3216d29bef63cddd6bfc25a967b534605af232 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,15 @@
 
                                   Changelog
 
+Daniel (6 December 2006)
+- Sebastien Willemijns reported bug #1603712
+  (http://curl.haxx.se/bug/view.cgi?id=1603712) which is about connections
+  getting cut off prematurely when --limit-rate is used. While I found no such
+  problems in my tests nor in my reading of the code, I found that the
+  --limit-rate code was severly flawed (since it was moved into the lib, since
+  7.15.5) when used with the easy interface and it didn't work as documented
+  so I reworked it somewhat and now it works for my tests.
+
 Daniel (5 December 2006)
 - Stefan Krause pointed out a compiler warning with a picky MSCV compiler when
   passing a curl_off_t argument to the Curl_read_rewind() function which takes

diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 2cb16a6200370a60cab7f9fce3c83591b28e8c39..fc1784623894e8fd3f83e721639ab95b51fcdd4c 100644 (file)
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -32,6 +32,7 @@ This release includes the following bugfixes:
  o curl_getdate() could be off one hour for TZ time zones with DST, on windows
  o CURLOPT_FORBID_REUSE works again
  o CURLOPT_MAXCONNECTS set to zero caused libcurl to SIGSEGV
+ o rate limiting works better
 
 Other curl-related news:
 
@@ -50,6 +51,6 @@ advice from friends like these:
  James Housley, Olaf Stueben, Yang Tse, Gisle Vanem, Bradford Bruce,
  Ciprian Badescu, Dmitriy Sergeyev, Nir Soffer, Venkat Akella, Toon Verwaest,
  Matt Witherspoon, Alexey Simak, Martin Skinner, Sh Diao, Jared Lundell,
- Stefan Krause
+ Stefan Krause, Sebastien Willemijns
 
         Thanks! (and sorry if I forgot to mention someone)

diff --git a/TODO-RELEASE b/TODO-RELEASE
index 4b6e4e50c64f3e8528e779ab83eecce7695f974e..97b8bfe19d075a8312aec544fd71c621aa65f854 100644 (file)
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -3,9 +3,6 @@ To get fixed in 7.16.1 (planned release: January 2007)
 
 69 - Jeff Pohlmeyer's crashing case (posted 17 nov 2006)
 
-71 - rate-limit when transferring files (using the easy interface) is
-     completely broken, bug #1603712. Patch pending commit on feedback.
-
 75 - "copying everything downloaded" (posted 11 nov 2006).
 
 77 -

diff --git a/lib/transfer.c b/lib/transfer.c
index 4406aeb4fde1154d91b1df5e86cc2199fa30f9c1..da770ca8acc9e6c445664baeae5a22df63a42d40 100644 (file)
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -311,12 +311,15 @@ CURLcode Curl_readwrite(struct connectdata *conn,
 
   curl_off_t contentlength;
 
-  if(k->keepon & KEEP_READ)
+  /* only use the proper socket if the *_HOLD bit is not set simultaneously as
+     then we are in rate limiting state in that transfer direction */
+
+  if((k->keepon & (KEEP_READ|KEEP_READ_HOLD)) == KEEP_READ)
     fd_read = conn->sockfd;
   else
     fd_read = CURL_SOCKET_BAD;
 
-  if(k->keepon & KEEP_WRITE)
+  if((k->keepon & (KEEP_WRITE|KEEP_WRITE_HOLD)) == KEEP_WRITE)
     fd_write = conn->writesockfd;
   else
     fd_write = CURL_SOCKET_BAD;
@@ -1530,7 +1533,7 @@ CURLcode Curl_readwrite(struct connectdata *conn,
   }
 
   /* Now update the "done" boolean we return */
-  *done = (bool)(0 == k->keepon);
+  *done = (bool)(0 == (k->keepon&(KEEP_READ|KEEP_WRITE)));
 
   return CURLE_OK;
 }
@@ -1699,37 +1702,40 @@ Transfer(struct connectdata *conn)
   while (!done) {
     curl_socket_t fd_read;
     curl_socket_t fd_write;
-    int interval_ms;
-
-    interval_ms = 1 * 1000;
 
     /* limit-rate logic: if speed exceeds threshold, then do not include fd in
-       select set */
-    if ( (data->set.max_send_speed > 0) &&
-         (data->progress.ulspeed > data->set.max_send_speed) )  {
-      fd_write = CURL_SOCKET_BAD;
-      Curl_pgrsUpdate(conn);
+       select set. The current speed is recalculated in each Curl_readwrite()
+       call */
+    if ((k->keepon & KEEP_WRITE) &&
+        (!data->set.max_send_speed ||
+         (data->progress.ulspeed < data->set.max_send_speed) )) {
+      fd_write = conn->writesockfd;
+      k->keepon &= ~KEEP_WRITE_HOLD;
     }
     else {
+      fd_write = CURL_SOCKET_BAD;
       if(k->keepon & KEEP_WRITE)
-        fd_write = conn->writesockfd;
-      else
-        fd_write = CURL_SOCKET_BAD;
+        k->keepon |= KEEP_WRITE_HOLD; /* hold it */
     }
 
-    if ( (data->set.max_recv_speed > 0) &&
-         (data->progress.dlspeed > data->set.max_recv_speed) ) {
-      fd_read = CURL_SOCKET_BAD;
-      Curl_pgrsUpdate(conn);
+    if ((k->keepon & KEEP_READ) &&
+        (!data->set.max_recv_speed ||
+         (data->progress.dlspeed < data->set.max_recv_speed)) ) {
+      fd_read = conn->sockfd;
+      k->keepon &= ~KEEP_READ_HOLD;
     }
     else {
+      fd_read = CURL_SOCKET_BAD;
       if(k->keepon & KEEP_READ)
-        fd_read = conn->sockfd;
-      else
-        fd_read = CURL_SOCKET_BAD;
+        k->keepon |= KEEP_READ_HOLD; /* hold it */
     }
 
-    switch (Curl_select(fd_read, fd_write, interval_ms)) {
+    /* The *_HOLD logic is necessary since even though there might be no
+       traffic during the select interval, we still call Curl_readwrite() for
+       the timeout case and if we limit transfer speed we must make sure that
+       this function doesn't transfer anything while in HOLD status. */
+
+    switch (Curl_select(fd_read, fd_write, 1000)) {
     case -1: /* select() error, stop reading */
 #ifdef EINTR
       /* The EINTR is not serious, and it seems you might get this more

diff --git a/lib/urldata.h b/lib/urldata.h
index f16cf10bc40e44b7836fc1a271caeab98165e899..6baf3dba7f743594c0b8e34f3b4717c293f3f8ff 100644 (file)
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -503,12 +503,14 @@ struct hostname {
 /*
  * Flags on the keepon member of the Curl_transfer_keeper
  */
-enum {
-  KEEP_NONE,
-  KEEP_READ,
-  KEEP_WRITE
-};
 
+#define KEEP_NONE  0
+#define KEEP_READ  1      /* there is or may be data to read */
+#define KEEP_WRITE 2      /* there is or may be data to write */
+#define KEEP_READ_HOLD 4  /* when set, no reading should be done but there
+                             might still be data to read */
+#define KEEP_WRITE_HOLD 8 /* when set, no writing should be done but there
+                             might still be data to write */
 
 /*
  * This struct is all the previously local variables from Curl_perform() moved