Window adjustment during channel read operation

[migueldeicaza]

Hello,

I am not exactly sure that this is a bug report, a misuse on my part, or a design flaw.

I use libssh2 with an async networking stack, and I have to emulate the behavior of send/recv that libssh2 relies on for it to work (for those interested, it is Apple's Network stack, and the NWConnection API). This works almost all of the time, but I have found that in some very odd circumstances, one of my packets never arrives to the other end, and the problem is a combination of my emulation, but also on libssh2 design.

Since libssh2 does not provide a mechanism to know when there is data available (the poll API while present, is flagged as deprecated and having corner cases) I resort to "poking" all my active channels when new data comes from the network in the form of calling libssh2_channel_read_ex on both the standard output and the auxiliary output. These APIs, rather than returning "Hey no data was available" when no data is available, return LIBSSH2_ERROR_EAGAIN.

In general, my stack deals with LIBSSH2_ERROR_EAGAIN by retrying those operations, but in this case, you really should just move on, because no more data might be coming at all. The remote end might genuinely not have anything to return.

The challenge is that libssh2_channel_read_ex might decide internally that the SSH read window is not big enough, and send a packet to adjust the window size (it is actually _libssh2_channel_read that does this, an internal function). And it will return dutifully a LIBSSH2_ERROR_EAGAIN, which will be propagated back to the user - but as a user of libssh2_channel_read_ex I really have no way of knowing if this return code means "Please retry, I am negotiating a larger window", or "No more data is available, carry on".

The side effect of not retrying the operation is that my emulation for send/recv on top of an async stack will reply to the next call from the networking stack that tries to write with the results from the previously queued operation, rather than queuing the next operation - and this is where I experience the packet loss. Without retrying the original libssh2_channel_read_ex that would trigger the window adjustment to match the retry, I end up with a broken state.

I think there are a range of possible solutions here:

(a) Perhaps I do not understand what good window and buffer sizes are. I have tried 2 megs, 1 meg, 8 meg windows, but they seem to all be adjusted dynamically. I have tried 1k to 32k buffer sizes. They all lead to scenarios where the window is still renegotiated in the middle of a read.

(b) Perhaps window changes are not necessary - I am not an expert on the protocol, but perhaps the window size negotiation is a nice-to-have, rather than a must-have, and I could just comment out the code that does the window negotiation inside libssh2_channel_read_ex

(c) If window adjustment is mandatory, perhaps libssh2 should surface an API that could be probed for both he heuristic used, and the new size set: libssh2_channel_window_get_heuristic (int buflen) and libssh2_channel_set_acceptable_window_size (int buflen). The current implemented heuristic is remote.window_size < (remote.window_size_initial / 4 * 3 + bufSize), and the adjusted version is remote.window_size_initial + buflen - remote.window_size. I suggest exposing those APIs for the sake of exposing the policy, so applications could react to this.

(d) Perhaps libssh2_channel_read_ex should return a specific code to signal that there is no data available, rather than LIBSSH2_ERROR_EAGAIN