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

libssh: SFTP transfer: slow progress meter update #11020

Closed
AndreasHuebner-ae opened this issue Apr 25, 2023 · 6 comments
Closed

libssh: SFTP transfer: slow progress meter update #11020

AndreasHuebner-ae opened this issue Apr 25, 2023 · 6 comments
Assignees
Labels

Comments

@AndreasHuebner-ae
Copy link

I've built curl 7.87.0 with libssh 0.10.4 for sftp support and I'm trying to download a 30 MiB file with a relatively slow bandwith of ~ 10 Mbit/s.

The update rate of the progress meter is then something like once every 4 seconds.
(To someone who doesn't know what's going on, the download appears to be stalling at first. No progress at all until ~ 6.5 MiB are transfered.)

It works fine when curl is built with libssh2. (Progress meter updates at least once every second.)

When looking into this I came to the conclusion, that the sftp transfer with libssh is done in blocking mode.
That means the do-while loop in readwrite_data() (lib/transfer.c) runs until maxloops are exceeded. So depending on buffer size, there would be no updating on the progress meter for a long time.

Is there any specific reason why libssh is used in blocking mode for this?
Curl seems to prefer nonblocking mode in general.
I tried to fix this by adding sftp_file_set_nonblocking(sshc->sftp_file) after sftp_open() and as far as I can tell this works without a problem. (No thorough testing done yet, though.)

@bagder
Copy link
Member

bagder commented Apr 25, 2023

It was done that way by the team that wrote the support. I don't know why but I also know it causes problems (see #8632) as everything internally in libcurl is meant to always be non-blocking.

@bagder
Copy link
Member

bagder commented Apr 25, 2023

@nmav do you recall maybe?

@AndreasHuebner-ae
Copy link
Author

Thanks for the quick reply.

The weird thing for me is that the sftp code is clearly prepared to deal with the non-blocking mode.
Otherwise, it would not make much sense to check for SSH_AGAIN like in this part:

      nread = sftp_async_read(conn->proto.sshc.sftp_file,
                              mem, (uint32_t)len,
                              conn->proto.sshc.sftp_file_index);

      myssh_block2waitfor(conn, (nread == SSH_AGAIN)?TRUE:FALSE);

      if(nread == SSH_AGAIN) {
        *err = CURLE_AGAIN;
        return -1;
      } 

@nmav
Copy link
Contributor

nmav commented Apr 25, 2023

What I remember is that the libssh2 backend was supporting async but libssh not at the time. As I didn't know how the libssh async/non-blocking mode will work (when developed) the curl code was modeled similar to libssh2. I do not know if libssh today supports non-blocking mode for sftp transfers.

@AndreasHuebner-ae
Copy link
Author

I found the post on the libssh mailing list, that is refering SCP, which I have not looked at yet.
For SFTP, I'm quite certain that it works. (for reading)
Looking at the Curl_read() calls on downloads, the behaviour is similar to libssh2 now.
This is the patch I used (diff to current master):

--- a/lib/vssh/libssh.c
+++ b/lib/vssh/libssh.c
@@ -1597,6 +1597,7 @@ static CURLcode myssh_statemach_act(struct Curl_easy *data, bool *block)
         MOVE_TO_SFTP_CLOSE_STATE();
         break;
       }
+      sftp_file_set_nonblocking(sshc->sftp_file);

       state(data, SSH_SFTP_DOWNLOAD_STAT);
       break;

But maybe someone else should test it as well. :)

@bagder
Copy link
Member

bagder commented Apr 27, 2023

But maybe someone else should test it as well. :)

The function is documented in a single brief line with no explanation whatsoever. It is also SFTP-specific so while it addresses this exact issue the same problem remains for SCP.

Sure I'm biased, but man @libssh this is lame.

bagder added a commit that referenced this issue Apr 27, 2023
Reported-by: Andreas Huebner
Fixes #11020
@bagder bagder self-assigned this Apr 27, 2023
@bagder bagder closed this as completed in ff67da5 Apr 27, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
Reported-by: Andreas Huebner
Fixes curl#11020
Closes curl#11039
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants