Daniel Stenberg wrote:
>> when send_existing() has fully transferred the remaining outgoing data,
>> the flag LIBSSH2_SESSION_BLOCK_OUTBOUND should be clear.
>
> Hm, yes. I agree and that's why the code originally did that!
>
> However, it seems there was some problem with that approach and the
> commit 7317edab61d2179febc38a2c2c4da0b951d74cbc by Peter Stuge
> changed that behavior.
That commit improved the previous state, but wasn't a full fix.
Thanks to Sebastien for reporting this problem!
Please try the attached patch.
I'll push it if it solves the problem you are seeing. (Please also
describe the circumstance that exposed this problem for you.)
Maybe this can help those curious about what is going on here:
See the code in _transport_write() after the call to send_existing().
Note the return value for send_existing() and how *ret is used.
send_existing() returns PACKET_NONE (0) when it has sent the final
part of a packet, and before that it always returns PACKET_EAGAIN or
an actual error. *ret is set to 1 when there are parts of a packet
remaining in (=before) this call to send_existing().
The if() immediately following the call to send_existing() in current
git makes _transport_write() return right away either on error in
send_existing() (good!) or when send_existing() finished sending a
packet (bad, must clear flag first!).
The only situation in which the flag is cleared, is if this call to
_transport_write() is the first call for a new packet.
The attached patch should split this up properly.
> I'm trying to read the commit message and think through the code to
> understand why that change was made, but I can't quite get it.
Read _transport_write() and send_existing() over and over. It's
pretty convoluted, again because of how the API for _transport_write()
works, but I'm sure you'll get it. Feel free to ask questions if we
can help.
> My analysis is instead that send_existing() should set the
> LIBSSH2_SESSION_BLOCK_OUTBOUND bit for the case when only a piece of
> the buffer was sent and it returns PACKET_EAGAIN - in the last line
> of the function. As otherwise, it will return EAGAIN without a
> direction bitmask set and that's not good.
>
> It signals to the caller that it wants to be called again and it
> sets for what direction it wants the socket to be ready to get
> called.
>
> Comments anyone?
Explicitly setting the flag not only on send() error but also on
send() success, instead of clearing the flag after finishing, would
work just as well but may not make a significant difference?
The question is if the bit can be cleared by someone else, between
calls to libssh2_transport_write().
If it can be cleared, then send_existing() should always set the
flag as you say.
If noone else will clear the bit, then it's fine for send_existing()
to just not touch the bit, and the setting of the bit in the error
code path could then be removed.
Keep in mind that send_existing() only sends the second part of a
packet, and since _transport_write() already has set the flag on
line 827 (when it sends out the first part) there's no point in
setting it again as long as the flag isn't cleared until the whole
packet has been sent out.
//Peter
Received on 2010-03-28