From 840e796aa9c12a94bec90ee2a0bd49ee3163deb1 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Wed, 6 Dec 2006 09:37:40 +0000 Subject: 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. --- CHANGES | 9 +++++++++ RELEASE-NOTES | 3 ++- TODO-RELEASE | 3 --- lib/transfer.c | 50 ++++++++++++++++++++++++++++---------------------- lib/urldata.h | 12 +++++++----- 5 files changed, 46 insertions(+), 31 deletions(-) diff --git a/CHANGES b/CHANGES index 60828fd3d..4d3216d29 100644 --- 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 2cb16a620..fc1784623 100644 --- 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 4b6e4e50c..97b8bfe19 100644 --- 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 4406aeb4f..da770ca8a 100644 --- 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 f16cf10bc..6baf3dba7 100644 --- 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 -- cgit v1.2.3