Re: [PATCH] socket: do not enable POLLOUT for empty out buffer

[Aris Adamantiadis <aris@xxxxxxxxxxxx>]

On 2/08/17 17:14, Nikolay wrote:
> Thank you, Aris.
>
>> No problem, as I understand you're using revents as a kind of a flag to
>> detect when the output buffer is totally empty. It's not exactly how it
>> was designed :)
>
> = No, I do understand that POLLOUT is either a
> connect-attempt-finished or outgoing-buffer-available event.
> This is why I suppose the patch - in my mind we don't need to wait for
> POLLOUT if we have already passed all the data to send(). I do not
> quite understand the reason why we need to wait for POLLOUT
> unconditionally...
> POLLOUT will only mean that there is space available in output buffer.
>
The idea is not that we "wait", but we pass the flag to the kind of
events we're interested in. Usually on the next main loop iteration,
poll returns with POLLOUT and we can set write_wont_block=1. We do this
in order to, in future, be able to send() the next packet without having
to wait for a main loop iteration. In the end we do the same amount of
poll operations. We cannot send() anything without being sure the write
operation will not block.

> But I do not analyze revent value in socket callback function. I
> simply check if session is established and do ssh_channel_select() or
> ssh_connect()/ssh_auth...() otherwise.
We probably need better ways of querying the ssh session to know in what
state it is (connecting/authenticating/disconnecting/errored)
>
>
>> I'm not sure I fully understand the sequence of actions here. Are we
>> somewhere in (backtrace)
>> ssh_event_dopoll()
>> ____ssh_socket_event_callback()
>> ________ssh_channel_select()
>
> = Yes, backtrace seems like the one I have in mind.
>
>> Are the parameters to these functions SSH sessions? Because if so you
>> may enter in reentrancy zone and you may have unexpected results.
> = I only pass session socket to ssh_event_dopoll().
> But you are possibly correct, as ssh_channel_select() seems to be
> working with the channels' sessions' sockets.
Actually from inside certain callbacks, some ssh functions will not
work, like functions that expect reading something additional from the
network. A good example is ssh_channel_open() in blocking mode.
>
>
>> The best way to handle the spurious POLLOUT is to add the current ssh
>> session into an ssh_event
>> LIBSSH_API int ssh_event_add_session(ssh_event event, ssh_session
>> session);
>> and then call ssh_event_dopoll() with timeout=0
>>
>> I'm not sure how it would fit into your architecture though. Maybe you
>> should use the channel callbacks instead of ssh_channel_select() inside
>> of global socket callbacks.
>
> Yes, I did try to use ssh_event_add_session() and set some callbacks.
> But I strongly need it all to work in non-blocking mode. But the
> documentation for ssh_event_add_session() says:
> > remove the poll handle from session and assign them to a event, when
> used in *blocking mode*
Normally the API is ok to use in non blocking mode, actually it makes no
sense to use blocking/synchronous APIs in an asynchronous design.
I just looked at the code again and I have no idea why it even mentions
blocking mode. I think it's because every synchronous API calls you will
make on a session you have added to an event handle will impact the
other sessions in that same event handle, because if libssh has to do a
synchronous recv() on SSH session 1, it has to run ssh_event_dopoll() on
the event structure that contains ssh sessions 1 and 2.
If your code is properly using callbacks it should work.
>
>
> I think it's the reason I failed with using ssh_event_add_session()
> and session callbacks. I did get some SIGSEGV during those experiments.
>
Maybe reentrancy issues? Send me a few backtraces, they can help.
> I'm connecting in the following way:
>
> if (session_not_connected && ssh_connect() == SSH_AGAIN)
>   return to eventloop and wait for event on session socket, then run
> ssh_connect() again
Looks fine. The best would be to use a session_connected callbacks that
we still haven't implemented.
>
>
> My thread should not block in waiting for particular
> socket/session/channel I/O. I should be able to connect to multiple
> ssh server in non-blocking async mode.
I agree, libssh was designed with this use case in mind. But we have
unfortunately probably missed a few corner cases because we haven't
written programs that do this.
>
>
> Best regards, Nikolay Karikh
>
>
>