[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


Hello, Aris.

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 failed to do it with callbacks & non-blocking mode.

The scenario is:

session = ssh_new();
*ssh_set_blocking(session, 0);**
*ssh_event_add_session(eventloop, session);
ssh_options_set(session, ...) ...;
struct ssh_callbacks_struct cb;
memset(&cb, 0, sizeof(ssh_callbacks_struct));
cb.userdata = this;
cb.connect_status_function = cb_connect_status_function;
cb.auth_function = cb_auth_function;
cb.log_function = cb_log_function;
ssh_callbacks_init(&cb);
ssh_set_callbacks(session, &cb);
ssh_connect(session);
...
while (1) {
  ssh_event_dopoll(eventloop, 500);
}

Log output is:
09:10:54.710 cb_log_callback: ssh_connect: libssh 0.7.0 (c) 2003-2014 Aris Adamantiadis, Andreas Schneider, and libssh contributors. Distributed under the LGPL, please refer to COPYING file for information about your rights, using threading threads_noop 09:10:54.710 cb_log_callback: ssh_socket_connect: Nonblocking connection socket: 23
09:10:54.710 cb_connect_status_function: 0.200000
09:10:54.710 cb_log_callback: ssh_connect: Socket connecting, now waiting for the callbacks to work 09:10:54.710 cb_log_callback: socket_callback_connected: Socket connection callback: 1 (0)

And that's it. No more calls to my callbacks.
If I make the session blocking:
*ssh_set_blocking(session, 1);**
*
Then it gets done right:

09:13:27.617 cb_log_callback: ssh_socket_connect: Nonblocking connection socket: 23
09:13:27.617 cb_connect_status_function: 0.200000
09:13:27.617 cb_log_callback: ssh_connect: Socket connecting, now waiting for the callbacks to work 09:13:27.617 cb_log_callback: socket_callback_connected: Socket connection callback: 1 (0)
09:13:27.621 cb_connect_status_function: 0.400000
09:13:27.621 cb_log_callback: ssh_client_connection_callback: SSH server banner: SSH-2.0-OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8 09:13:27.621 cb_log_callback: ssh_analyze_banner: Analyzing banner: SSH-2.0-OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8 09:13:27.621 cb_log_callback: ssh_analyze_banner: We are talking to an OpenSSH client version: 6.6 (60600)
09:13:27.621 cb_connect_status_function: 0.500000
09:13:27.622 cb_connect_status_function: 0.600000
09:13:27.623 cb_connect_status_function: 0.800000
09:13:27.666 cb_log_callback: ssh_packet_dh_reply: Received SSH_KEXDH_REPLY
09:13:27.676 cb_log_callback: ssh_client_curve25519_reply: SSH_MSG_NEWKEYS sent
09:13:27.676 cb_log_callback: ssh_packet_newkeys: Received SSH_MSG_NEWKEYS
09:13:27.677 cb_log_callback: ssh_packet_newkeys: Signature verified and valid
09:13:27.677 cb_connect_status_function: 1.000000

But in blocking mode during ssh_connect() call all other sockets are not getting their callbacks executed. I also need to check something periodically in main loop, so blocking mode doesn't seem like an option for me.

Am I doing something wrong way?

Best regards, Nikolay Karikh.


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






Attachment: smime.p7s
Description: Криптографическая подпись S/MIME


Archive administrator: postmaster@lists.cynapses.org