Corrected non-blocking code and fixed sftp_RW_nonblock.c#1812
Conversation
There was a problem hiding this comment.
Pull request overview
Updates libssh2 example programs to behave correctly in non-blocking mode by retrying operations that return LIBSSH2_ERROR_EAGAIN and by performing more robust channel/session/SFTP teardown. This aligns the examples with the library’s non-blocking API contract and fixes the previously broken sftp_RW_nonblock.c behavior.
Changes:
- Added/expanded
waitsocket()+EAGAINretry loops around handshake/auth and various libssh2 calls in multiple examples. - Made shutdown paths non-blocking-safe (EOF/close/free/disconnect/free loops).
- Fixed
sftp_RW_nonblock.cupload logic to avoid looping on zero/partial writes and to handle larger transfers.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| example/tcpip-forward.c | Adds waitsocket() and makes channel/listener/session teardown retry on EAGAIN. |
| example/ssh2_exec.c | Initializes channel to NULL; adds waitsocket() on EAGAIN for handshake/auth and shutdown. |
| example/ssh2_echo.c | Initializes channel to NULL; adds waitsocket() on EAGAIN for handshake/auth and session teardown. |
| example/ssh2_agent_forwarding.c | Initializes channel to NULL; makes channel/session teardown retry on EAGAIN. |
| example/sftpdir_nonblock.c | Adds waitsocket() and EAGAIN handling for handshake/auth/SFTP init/opendir/readdir and shutdown. |
| example/sftp_write_sliding.c | Adds waitsocket() to handshake/auth; modifies SFTP init/open loops and shutdown to retry on EAGAIN. |
| example/sftp_nonblock.c | Adds waitsocket() for handshake/auth/publickey; changes shutdown to retry SFTP/session teardown on EAGAIN. |
| example/sftp_mkdir_nonblock.c | Adds waitsocket() and wraps handshake/auth/mkdir and teardown with EAGAIN retry loops. |
| example/sftp_RW_nonblock.c | Fixes non-blocking handling around handshake/auth/open/write/close/shutdown and improves write loop correctness. |
| example/scp_write_nonblock.c | Adds waitsocket() to handshake/auth/scp_send64; makes channel/session teardown retry on EAGAIN. |
| example/scp_nonblock.c | Adds waitsocket() to handshake/auth/scp_recv2; makes channel/session teardown retry on EAGAIN. |
| example/direct_tcpip.c | Adds header comment + waitsocket(); makes channel/session teardown retry on EAGAIN. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(!sftp_session) { | ||
| if(libssh2_session_last_errno(session) == LIBSSH2_ERROR_EAGAIN) { | ||
| fprintf(stderr, "non-blocking init\n"); | ||
| waitsocket(sock, session); /* now we wait */ | ||
| } | ||
| else { | ||
| fprintf(stderr, "Unable to init SFTP session\n"); |
There was a problem hiding this comment.
In the libssh2_sftp_open() retry loop, the code checks if(!sftp_session) instead of if(!sftp_handle). This prevents proper handling of EAGAIN/error cases from the open call and can lead to a tight infinite loop when open fails. The condition (and associated error message) should be based on the sftp_handle result.
| if(!sftp_session) { | |
| if(libssh2_session_last_errno(session) == LIBSSH2_ERROR_EAGAIN) { | |
| fprintf(stderr, "non-blocking init\n"); | |
| waitsocket(sock, session); /* now we wait */ | |
| } | |
| else { | |
| fprintf(stderr, "Unable to init SFTP session\n"); | |
| if(!sftp_handle) { | |
| if(libssh2_session_last_errno(session) == LIBSSH2_ERROR_EAGAIN) { | |
| fprintf(stderr, "non-blocking open\n"); | |
| waitsocket(sock, session); /* now we wait */ | |
| } | |
| else { | |
| fprintf(stderr, "Unable to open file with SFTP\n"); |
|
|
||
| shutdown: | ||
| if(sftp_handle) | ||
| while(libssh2_sftp_closedir(sftp_handle) == LIBSSH2_ERROR_EAGAIN) |
There was a problem hiding this comment.
sftp_handle in this example is created via libssh2_sftp_open(...) (file handle), but shutdown now calls libssh2_sftp_closedir(sftp_handle). That is the directory-handle close API and is not the correct counterpart for libssh2_sftp_open; it should use the file-handle close API instead (still handling LIBSSH2_ERROR_EAGAIN as needed).
| while(libssh2_sftp_closedir(sftp_handle) == LIBSSH2_ERROR_EAGAIN) | |
| while(libssh2_sftp_close(sftp_handle) == LIBSSH2_ERROR_EAGAIN) |
| libssh2_session_last_errno(session) != LIBSSH2_ERROR_EAGAIN) { | ||
| fprintf(stderr, "Unable to init SFTP session\n"); | ||
| goto shutdown; | ||
| if(!sftp_handle) { |
There was a problem hiding this comment.
In the SFTP init loop, the null-check is mistakenly using sftp_handle instead of sftp_session. Since sftp_handle hasn’t been set yet, this path will treat even successful libssh2_sftp_init() calls as failures (or wait unnecessarily) and can jump to shutdown incorrectly. Update the condition to check sftp_session here.
| if(!sftp_handle) { | |
| if(!sftp_session) { |
All non-blocking examples are not handling EAGAIN while close channels and sessions.
Also addad waitsocket() when EAGIN is returned.
sftp_RW_nonblock.c was not working, looping when libssh2_sftp_write() returned zero and not hanling file over the size of 1000 bytes.