Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix progress meter in parallel mode #6840

Closed
wants to merge 3 commits into from

Conversation

@Cherish98
Copy link
Contributor

@Cherish98 Cherish98 commented Apr 2, 2021

Make sure the total amount of DL/UL bytes are counted before the transfer finalizes. Otherwise if a transfer finishes too quick, its total numbers may not be added, and results in a DL%/UL% that goes above 100%.

Cherish98 added 3 commits Dec 23, 2020
Make sure the total amount of DL/UL bytes are counted before the transfer finalizes. Otherwise if a transfer finishes too quick, its total numbers are not added, and results in a DL%/UL% that goes above 100%.
@jay
Copy link
Member

@jay jay commented Apr 2, 2021

Is it reproducible? I'm trying to figure out why progress_meter wouldn't be called before progress_finalize, or if it is then why it is not updating those variables.

curl/src/tool_operate.c

Lines 2279 to 2331 in 3266b35

while(!mcode && (still_running || more_transfers)) {
mcode = curl_multi_poll(multi, NULL, 0, 1000, NULL);
if(!mcode)
mcode = curl_multi_perform(multi, &still_running);
progress_meter(global, &start, FALSE);
if(!mcode) {
int rc;
CURLMsg *msg;
bool checkmore = FALSE;
do {
msg = curl_multi_info_read(multi, &rc);
if(msg) {
bool retry;
long delay;
struct per_transfer *ended;
CURL *easy = msg->easy_handle;
result = msg->data.result;
curl_easy_getinfo(easy, CURLINFO_PRIVATE, (void *)&ended);
curl_multi_remove_handle(multi, easy);
result = post_per_transfer(global, ended, result, &retry, &delay);
progress_finalize(ended); /* before it goes away */
all_added--; /* one fewer added */
checkmore = TRUE;
if(retry) {
ended->added = FALSE; /* add it again */
/* we delay retries in full integer seconds only */
ended->startat = delay ? time(NULL) + delay/1000 : 0;
}
else
(void)del_per_transfer(ended);
}
} while(msg);
if(!checkmore) {
time_t tock = time(NULL);
if(tick != tock) {
checkmore = TRUE;
tick = tock;
}
}
if(checkmore) {
/* one or more transfers completed, add more! */
(void)add_parallel_transfers(global, multi, share,
&more_transfers,
&added_transfers);
if(added_transfers)
/* we added new ones, make sure the loop doesn't exit yet */
still_running = 1;
}
}
}

I do see progress_meter needs a time difference of at least 500 ms if not the final round. Perhaps there should be a check for the initial round as well.

@jay
Copy link
Member

@jay jay commented Apr 2, 2021

try this

diff --git a/src/tool_progress.c b/src/tool_progress.c
index 7db6df9..20edaeb 100644
--- a/src/tool_progress.c
+++ b/src/tool_progress.c
@@ -174,13 +174,7 @@ bool progress_meter(struct GlobalConfig *global,
   now = tvnow();
   diff = tvdiff(now, stamp);
 
-  if(!header) {
-    header = TRUE;
-    fputs("DL% UL%  Dled  Uled  Xfers  Live   Qd "
-          "Total     Current  Left    Speed\n",
-          global->errors);
-  }
-  if(final || (diff > 500)) {
+  if(!header || final || (diff > 500)) {
     char time_left[10];
     char time_total[10];
     char time_spent[10];
@@ -199,6 +193,13 @@ bool progress_meter(struct GlobalConfig *global,
     unsigned int i;
     stamp = now;
 
+    if(!header) {
+      header = TRUE;
+      fputs("DL% UL%  Dled  Uled  Xfers  Live   Qd "
+            "Total     Current  Left    Speed\n",
+            global->errors);
+    }
+
     /* first add the amounts of the already completed transfers */
     all_dlnow += all_dlalready;
     all_ulnow += all_ulalready;

@Cherish98
Copy link
Contributor Author

@Cherish98 Cherish98 commented Apr 3, 2021

Is it reproducible?

Yes, I found it is easily reproduced when you make a lot of medium-sized transfers on a high-speed link. For example, I was downloading 200 files (5 MB each) from localhost.

$ curl -Z 127.1/xx{000..199} > /dev/null
DL% UL%  Dled  Uled  Xfers  Live   Qd Total     Current  Left    Speed
869 --  1000M     0   200     0     0 --:--:-- --:--:-- --:--:-- 1402M

$ curl -Z 127.1/xx{000..199} > /dev/null
DL% UL%  Dled  Uled  Xfers  Live   Qd Total     Current  Left    Speed
588 --  1000M     0   200     0     0 --:--:-- --:--:-- --:--:-- 1283M

If the files are too small (e.g., 1 MB each), the transfers will finish too quickly and DL% will remain --.

DL% UL%  Dled  Uled  Xfers  Live   Qd Total     Current  Left    Speed
--  --   200M     0   200     0     0 --:--:-- --:--:-- --:--:-- 1960M

And if the files are too big (e.g., 100 MB each), the progress is likely to be correct.

DL% UL%  Dled  Uled  Xfers  Live   Qd Total     Current  Left    Speed
100 --  19.5G     0   200     0     0  0:00:13  0:00:13 --:--:-- 1504M

YMMV, because it depends on the loopback interface speed, and in turn your CPU speed, etc.


I'm trying to figure out why progress_meter wouldn't be called before progress_finalize, or if it is then why it is not updating those variables.

Because when a transfer is finished, parallel_transfers() deletes the struct per_transfer before calling progress_meter(), and thus its stats need to be saved in progress_finalize(). That is why progress_finalize() existed in the first place.

progress_meter() is called periodically, and it may not catch a transfer's total bytes if the value was unknown during the last call, and the transfer is finished and deleted (i.e., lost) during the next call. (That is why the progress meter is correct when the file size is big enough.) So, you have to make sure progress_finalize() saves the value before it goes away.


try this

diff --git a/src/tool_progress.c b/src/tool_progress.c
index 7db6df9..20edaeb 100644
--- a/src/tool_progress.c
+++ b/src/tool_progress.c
@@ -174,13 +174,7 @@ bool progress_meter(struct GlobalConfig *global,
   now = tvnow();
   diff = tvdiff(now, stamp);
 
-  if(!header) {
-    header = TRUE;
-    fputs("DL% UL%  Dled  Uled  Xfers  Live   Qd "
-          "Total     Current  Left    Speed\n",
-          global->errors);
-  }
-  if(final || (diff > 500)) {
+  if(!header || final || (diff > 500)) {
     char time_left[10];
     char time_total[10];
     char time_spent[10];
@@ -199,6 +193,13 @@ bool progress_meter(struct GlobalConfig *global,
     unsigned int i;
     stamp = now;
 
+    if(!header) {
+      header = TRUE;
+      fputs("DL% UL%  Dled  Uled  Xfers  Live   Qd "
+            "Total     Current  Left    Speed\n",
+            global->errors);
+    }
+
     /* first add the amounts of the already completed transfers */
     all_dlnow += all_dlalready;
     all_ulnow += all_ulalready;

This does not fix the issue.

bagder
bagder approved these changes Apr 5, 2021
@bagder
Copy link
Member

@bagder bagder commented Apr 5, 2021

@jay you okay with this?

@jay jay closed this in 4b4401e Apr 6, 2021
@jay
Copy link
Member

@jay jay commented Apr 6, 2021

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants