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

-Z / --parallel breaks --fail-early --fail #6939

Closed
luke-clifton opened this issue Apr 23, 2021 · 10 comments
Closed

-Z / --parallel breaks --fail-early --fail #6939

luke-clifton opened this issue Apr 23, 2021 · 10 comments

Comments

@luke-clifton
Copy link

@luke-clifton luke-clifton commented Apr 23, 2021

I did this

curl --parallel-max 1 -Z --fail --fail-early https://curl.se/nothere[1-3]

I expected the following

Stop as soon as one as we know something failed, and return a non-0 exit code.

What actually happens

curl will attempt every single url, and then exit with a 0.

curl/libcurl version

I did it on master:

curl 7.77.0-DEV (x86_64-apple-darwin19.6.0) libcurl/7.77.0-DEV OpenSSL/1.1.1k
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: alt-svc AsynchDNS HSTS HTTPS-proxy IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets

But also other version.

operating system

Darwin BLAH 19.6.0 Darwin Kernel Version 19.6.0: Tue Jan 12 22:13:05 PST 2021; root:xnu-6153.141.16~1/RELEASE_X86_64 x86_64 i386 MacBookPro15,1 Darwin

@jay
Copy link
Member

@jay jay commented Apr 23, 2021

Ref: #6921

It might be a bug, I'm not sure myself. It seems --fail-early wasn't meant for parallel transfers. The transfers in progress would need to be properly cleaned up.

@luke-clifton
Copy link
Author

@luke-clifton luke-clifton commented Apr 23, 2021

Haha, OK. So I was a couple of days short on my master build. I see that the error code is not discarded anymore.

But the --fail-early (as you mention in your PR) is still an issue.

I'll agree that I'm not sure what the best thing to do would be: finish in-flight transfers, or close everything down. But I certainly feel that starting new transfers is not what should happen.

@luke-clifton luke-clifton changed the title -Z / --parallel breaks --fail-early and -f -Z / --parallel breaks --fail-early --fail Apr 23, 2021
@bagder
Copy link
Member

@bagder bagder commented Apr 23, 2021

It seems --fail-early wasn't meant for parallel transfers.

It was added long before we did parallel transfers and I think we just haven't adapted parallel mode properly to deal with it. I think it sounds like a proper bug...

@jay
Copy link
Member

@jay jay commented Apr 23, 2021

We could set a flag in the multi loop and then abort from the progress callback.

diff --git a/src/tool_progress.c b/src/tool_progress.c
index da6c2bc..4123e93 100644
--- a/src/tool_progress.c
+++ b/src/tool_progress.c
@@ -101,6 +101,9 @@ int xferinfo_cb(void *clientp,
   per->ultotal = ultotal;
   per->ulnow = ulnow;
 
+  if(per->abort)
+    return -1;
+
   if(config->readbusy) {
     config->readbusy = FALSE;
     curl_easy_pause(per->curl, CURLPAUSE_CONT);

@bagder
Copy link
Member

@bagder bagder commented Apr 23, 2021

We'd have to discuss what the option should do about other already started transfers...

@luke-clifton
Copy link
Author

@luke-clifton luke-clifton commented Apr 23, 2021

I'd say --fail-early should kill it as early as possible. Perhaps another --fail-gracefully option would allow transfers to finish. If you let --fail-early finish transfers, you are going to end up needing a --fail-really-early for the other case.

That being said, my usecase is 1000's of tiny transfers, so I don't care 🤷

@bagder
Copy link
Member

@bagder bagder commented Apr 23, 2021

I'm fine with killing off the already started ones too, as long as we document that this is to be expected when -Z is used with this option.

@jay
Copy link
Member

@jay jay commented Apr 23, 2021

We'd have to discuss what the option should do about other already started transfers...

Aborting from the progress callback would kill them off

@bagder
Copy link
Member

@bagder bagder commented Apr 29, 2021

@jay will you make a PR based on your proposed fix?

jay added a commit to jay/curl that referenced this issue Apr 29, 2021
(Draft.)

- Abort via progress callback to fail early on parallel transfers.

Fixes curl#6939
Closes #xxxx
@jay
Copy link
Member

@jay jay commented Apr 29, 2021

@jay will you make a PR based on your proposed fix?

#6984 to abort via progress callback but it would mean setting CURLOPT_NOPROGRESS to 0L.

> curld --parallel-max 1 -Z --fail --fail-early https://curl.se/nothere[1-3]

[1/3]: https://curl.se/nothere1 --> <stdout>
--_curl_--https://curl.se/nothere1
curl: (22) The requested URL returned error: 404
curl: (22) HTTP response code said error
> curld --fail --fail-early -Z https://httpbin.org/status/404 https://httpbin.org/delay/10
curl: (22) The requested URL returned error: 404
curl: (42) Callback aborted

I'm not sure what to do about the return code in the latter case or if it's ok to always set noprogress to 0. Have a look

edit: maybe for the return code skip setting aborted by callback if another error is already set

diff --git a/src/tool_operate.c b/src/tool_operate.c
index 8db6a46..5219eaa 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -2333,7 +2333,7 @@ static CURLcode parallel_transfers(struct GlobalConfig *global,
             ended->startat = delay ? time(NULL) + delay/1000 : 0;
           }
           else {
-            if(tres)
+            if(tres && (!result || tres != CURLE_ABORTED_BY_CALLBACK))
               result = tres;
             if(result && global->fail_early)
               wrapitup = TRUE;
@@ -2359,7 +2359,7 @@ static CURLcode parallel_transfers(struct GlobalConfig *global,
         CURLcode tres = add_parallel_transfers(global, multi, share,
                                                &more_transfers,
                                                &added_transfers);
-        if(tres)
+        if(tres && (!result || tres != CURLE_ABORTED_BY_CALLBACK))
           result = tres;
         if(added_transfers)
           /* we added new ones, make sure the loop doesn't exit yet */

(^^ I've added the above change to the draft)

jay added a commit to jay/curl that referenced this issue May 5, 2021
(Draft.)

- Abort via progress callback to fail early on parallel transfers.

Fixes curl#6939
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Jul 29, 2021
- Abort via progress callback to fail early during parallel transfers.

When a critical error occurs during a transfer (eg --fail-early
constraint) then other running transfers will be aborted via progress
callback and finish with error CURLE_ABORTED_BY_CALLBACK (42). In this
case, the callback error does not become the most recent error and a
custom error message is used for those transfers:

curld --fail --fail-early --parallel
https://httpbin.org/status/404 https://httpbin.org/delay/10

curl: (22) The requested URL returned error: 404
curl: (42) Transfer aborted due to critical error in another transfer

> echo %ERRORLEVEL%
22

Fixes curl#6939
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Jul 30, 2021
- Abort via progress callback to fail early during parallel transfers.

When a critical error occurs during a transfer (eg --fail-early
constraint) then other running transfers will be aborted via progress
callback and finish with error CURLE_ABORTED_BY_CALLBACK (42). In this
case, the callback error does not become the most recent error and a
custom error message is used for those transfers:

curld --fail --fail-early --parallel
https://httpbin.org/status/404 https://httpbin.org/delay/10

curl: (22) The requested URL returned error: 404
curl: (42) Transfer aborted due to critical error in another transfer

> echo %ERRORLEVEL%
22

Fixes curl#6939
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Jul 30, 2021
- Abort via progress callback to fail early during parallel transfers.

When a critical error occurs during a transfer (eg --fail-early
constraint) then other running transfers will be aborted via progress
callback and finish with error CURLE_ABORTED_BY_CALLBACK (42). In this
case, the callback error does not become the most recent error and a
custom error message is used for those transfers:

curld --fail --fail-early --parallel
https://httpbin.org/status/404 https://httpbin.org/delay/10

curl: (22) The requested URL returned error: 404
curl: (42) Transfer aborted due to critical error in another transfer

> echo %ERRORLEVEL%
22

Fixes curl#6939
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Aug 9, 2021
- Abort via progress callback to fail early during parallel transfers.

When a critical error occurs during a transfer (eg --fail-early
constraint) then other running transfers will be aborted via progress
callback and finish with error CURLE_ABORTED_BY_CALLBACK (42). In this
case, the callback error does not become the most recent error and a
custom error message is used for those transfers:

curld --fail --fail-early --parallel
https://httpbin.org/status/404 https://httpbin.org/delay/10

curl: (22) The requested URL returned error: 404
curl: (42) Transfer aborted due to critical error in another transfer

> echo %ERRORLEVEL%
22

Fixes curl#6939
Closes #xxxx
@jay jay closed this in b654fb4 Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants