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

Hangs on easy cleanup when aborted by callback #6333

Closed
Togtja opened this issue Dec 16, 2020 · 7 comments
Closed

Hangs on easy cleanup when aborted by callback #6333

Togtja opened this issue Dec 16, 2020 · 7 comments
Assignees

Comments

@Togtja
Copy link

Togtja commented Dec 16, 2020

I did this

Using CURLOPT_XFERINFODATA and CURLOPT_XFERINFOFUNCTION that send in a struct with an abort bool.
Where the function essentially has this:

    if (meta->abort) {
        return 1;
    }

The download is an sftp/ftp download

During the perform step it return CURLE_ABORTED_BY_CALLBACK

And then it goes to clean up, and it hangs there for about 2 minutes.
curl_easy_cleanup(curl);

I expected the following

That the cleanup would take a very short time, as it happens for most returns.

curl/libcurl version

curl 7.74.0 (Linux) libcurl/7.74.0 OpenSSL/1.1.1g zlib/1.2.11 libssh2/1.9.0_DEV
Release-Date: 2020-12-09
Protocols: dict file ftp ftps gopher http https imap imaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS HTTPS-proxy IPv6 Largefile NTLM SSL UnixSockets

I also tried this on 7.73, and same behaviour.

EDIT: Downloaded source-code.zip, instead of 7.74.0.zip, but fix that
EDIT 2: Forgot it ran it on my arm emulator, so fixed the versioning

operating system

Custom OS made using buildroot
Linux Togtja 4.20.0-OS_NAME #1 SMP PREEMPT Wed Dec 16 12:52:46 CET 2020 armv7l GNU/Linux

@Togtja
Copy link
Author

Togtja commented Dec 16, 2020

Running more tests, it seems to be very accurately stuck for 2 minutes.
From my log

2020-12-16 11:05:58.744416136;I;19831;easy cleanup
2020-12-16 11:07:58.746969519;I;19831;global cleanup

2020-12-16 11:18:47.769916181;I;21527;easy cleanup
2020-12-16 11:20:47.771713916;I;21527;global cleanup

2020-12-16 11:21:42.455842282;I;21915;easy cleanup
2020-12-16 11:23:42.456945581;I;21915;global cleanup

@bagder
Copy link
Member

bagder commented Dec 16, 2020

Can you create a small example source code for us that reproduces this?

@Togtja
Copy link
Author

Togtja commented Dec 17, 2020

Sorry for taking so long time to get back to you with a source code. I released that I actually compiled for ARM, on a custom Linux OS, forgot what it was based of, anyway the "uname -a" is:
Linux Togtja 4.20.0-OS_NAME #1 SMP PREEMPT Wed Dec 16 12:52:46 CET 2020 armv7l GNU/Linux

Where OS_NAME is what we have called the OS, uncertain if I can disclose it, but don't think it matters stoo much
Now, I have made a simple source code for this (in C++, as my C is very rusty):

size_t serverToFile(char* buffer, size_t size, size_t nmemb, void* null) {
    return size * nmemb;
}
struct prog {
    bool abort = false;
};

int progress_callback(prog* meta, curl_off_t _, curl_off_t _1, curl_off_t _2, curl_off_t _3) {
    if (meta->abort) {
        std::cout << "aborted\n";
        return 1;
    }
    return 0;
}

void curl_stuff(CURL* curl) {
    prog p;
    curl_easy_setopt(curl, CURLOPT_NOPROGRESS, false);
    curl_easy_setopt(curl, CURLOPT_XFERINFOFUNCTION, progress_callback);
    curl_easy_setopt(curl, CURLOPT_XFERINFODATA, &p);

    curl_easy_setopt(curl, CURLOPT_USERNAME, "root");
    curl_easy_setopt(curl, CURLOPT_PASSWORD, "root");

    auto url = "ftp://host/file.zip";
    curl_easy_setopt(curl, CURLOPT_URL, url);
    curl_easy_setopt(curl, CURLOPT_PORT, 21);

    curl_easy_setopt(curl, CURLOPT_MAX_RECV_SPEED_LARGE, 10000); // so it not downloaded too fast
    curl_easy_setopt(curl, CURLOPT_BUFFERSIZE, 10000);

    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, serverToFile);
    curl_easy_setopt(curl, CURLOPT_WRITEDATA, nullptr);

    curl_easy_setopt(curl, CURLOPT_RESUME_FROM_LARGE, 100);
    auto thread1 = std::thread([&p]() {
        std::this_thread::sleep_for(std::chrono::seconds(10));

        p.abort = true;
        return;
    });
    CURLcode res = curl_easy_perform(curl);

    thread1.join();
    std::cout << "response: " << curl_easy_strerror(res) << " for " << url << "\n";
}

int main(int argc, char** argv) {
    curl_global_init(CURL_GLOBAL_DEFAULT);
    CURL* curl = curl_easy_init();
    if (!curl) {
        // Failed to init curl
        curl_global_cleanup();
        std::cout << "failed to init curl, probably memory issues\n";
        return 0;
    }

    curl_stuff(curl);
    auto start = std::chrono::steady_clock::now();
    std::cout << "easy cleanup\n";
    curl_easy_cleanup(curl);
    auto time = std::chrono::steady_clock::now() - start;
    std::cout << "global cleanup easy took:" << std::chrono::duration_cast<std::chrono::seconds>(time).count() << "s\n";
    curl_global_cleanup();
    return 0;
}

Where it logs:

# ./downloader        
aborted
response: Operation was aborted by an application callback for ftp://host/file.zip
easy cleanup
global cleanup easy took:120s

@bagder bagder added the FTP label Dec 17, 2020
@bagder
Copy link
Member

bagder commented Dec 17, 2020

So to be clear, you see this problem with FTP only right? not SFTP .

@bagder bagder self-assigned this Dec 17, 2020
@Togtja
Copy link
Author

Togtja commented Dec 17, 2020

I thought it was an FTP and SFTP, but further testing seems to prove it ONLY happens on FTP.

bagder added a commit that referenced this issue Dec 17, 2020
When failing in TOOFAST, the multi_done() wasn't called so the same
cleanup and handling wasn't done like when it fails in PERFORM, which in
the case of FTP could mean that the control connection wouldn't be
marked as "dead" for the CURLE_ABORTED_BY_CALLBACK case. Which caused
ftp_disconnect() to use it to send "QUIT", which could end up waiting
for a response a long time before giving up!

Reported-by: Tomas Berger
Fixes #6333
@bagder
Copy link
Member

bagder commented Dec 17, 2020

I'm pretty sure my fix in #6337 solves this for you.

@Togtja
Copy link
Author

Togtja commented Dec 17, 2020

After compiling and running, it does indeed seem to have fixed it 👍
Thank you for such a fast PR!

@Togtja Togtja closed this as completed Dec 17, 2020
bagder added a commit that referenced this issue Dec 17, 2020
When failing in TOOFAST, the multi_done() wasn't called so the same
cleanup and handling wasn't done like when it fails in PERFORM, which in
the case of FTP could mean that the control connection wouldn't be
marked as "dead" for the CURLE_ABORTED_BY_CALLBACK case. Which caused
ftp_disconnect() to use it to send "QUIT", which could end up waiting
for a response a long time before giving up!

Reported-by: Tomas Berger
Fixes #6333
Closes #6337
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants