Configurable sftp timeout with libssh2 #10965
Conversation
|
Maybe this should use |
danielsilverstone-ct
force-pushed
the
configurable-sftp-timeout
branch
from
f8a64d9
to
ed63762
Compare
|
Regarding If you feel that it's still appropriate for this use-case then I can certainly massively simplify this patch. |
|
It also looks like my colleague didn't test building against a non-patched libssh2 too, so I will fix that momentarily. That bit will have to change regardless of the option we hang it off. |
lib/vssh/libssh2.c
Outdated
| /* Set the packet read timeout if the libssh2 version supports it */ | ||
| #ifdef HAVE_LIBSSH2_VERSION | ||
| if(libssh2_version(0x010B00)) { | ||
| libssh2_session_set_read_timeout(sshc->ssh_session, |
There was a problem hiding this comment.
libssh2_session_set_read_timeout should be:
libssh2_session_set_timeout
There was a problem hiding this comment.
See my comment a few minutes ago about not having the build tested against current libssh2 (this patch is for an as-yet unreleased feature of libssh2). I've now pushed a fix for this mis-build
danielsilverstone-ct
force-pushed
the
configurable-sftp-timeout
branch
from
ed63762
to
4b75fd7
Compare
|
Given the potential delay with waiting for libssh2 to be released, should I mark this PR as 'draft'? If I do, will that prevent CI from running? |
|
The missing |
I actually think they are very similar. They are about how long libcurl is allowed to wait for a "command response" from the server before considering it a failure. This "command response" is for a command that is done as one out of many to get the transfer going. Sure, FTP is very different than SFTP as protocols but it feels like this option is still a max time to allow for a server to respond to a query. So yes, I think we can re-use this option for SFTP (libssh2) and instead just documented that it only works for SFTP/libssh2 starting in version XXXX. I don't think we will manage to get this done and merged before 8.1.0 feature freeze (Thursday this week). |
|
OK, so I'll work on redoing this branch to trigger off Thank you for taking the time to think this over. |
Hook the new (1.11.0 or newer) libssh2 support for setting a read timeout into the SERVER_RESPONSE_TIMEOUT option. With this done, clients can use the standard curl response timeout setting to also control the time that libssh2 will wait for packets from a slow server. This is necessary to enable use of very slow SFTP servers. Signed-off-by: Daniel Silverstone <daniel.silverstone@codethink.co.uk>
danielsilverstone-ct
force-pushed
the
configurable-sftp-timeout
branch
from
4b75fd7
to
fd15235
Compare
|
I'm not sure that the failure in websockets could be caused by my change, but maybe I messed something up? |
|
@bagder Do you have any view on what might have caused that failing test? Should I rebase? |
No need, that's just a "normal" flaky CI job. Not your fault. |
|
Thanks! |
Hook the new (1.11.0 or newer) libssh2 support for setting a read timeout into the SERVER_RESPONSE_TIMEOUT option. With this done, clients can use the standard curl response timeout setting to also control the time that libssh2 will wait for packets from a slow server. This is necessary to enable use of very slow SFTP servers. Signed-off-by: Daniel Silverstone <daniel.silverstone@codethink.co.uk> Closes curl#10965
This set of changes provides support to configure the libssh2 read timeout, capability for which was introduced in libssh2/libssh2#892 and it is estimated will be released with libssh2 version 1.11.0.
The goal here is to enable users who have to work with very slow sftp servers to configure the timeout such that they don't end up with the default 60s timeout biting them when a server takes a long time to, for example, open a directory.
We have taken a guess that 1.11.0 will be released moderately soon, and thus projected that you will be unlikely to have released 8.1.0 in that time. Given that, we've noted version numbers on the docs etc. as this feature being in curl 8.1.0 but we can obviously tweak that if necessary.
I'd guess that it makes sense to not merge this until we're confident about libssh2's release timeline and can ensure numbers line up.
I am not sure how to usefully add a test to curl's testsuite for this because (a) libssh2 is not released with the patch yet and (b) I think it'd involve a patched sftp server to introduce some kind of delay, coupled with then a low timeout set in a test case. ie. it'd be quite awkward to contrive the scenario in the test suite. If you have ideas, then please let me know and I'll see what we can come up with.
Thanks,
D.