Please help review and test solution for SSHD-849

[Lyor Goldstein <lg...@apache.org>]

Here is the issue in a nutshell - a client might open an SSH tunnel, send
some data and close (normally) its side of the tunnel before the channel to
the other side has been successfully established and all data transmitted.
Currently a race condition may occur in such a scenario where the code
closes the channel while pending messages are still in transit.

The patch attempts to accumulate pending messages until channel is open
(successfully) and then flush them while maintaining their *order*. At the
same time, it attempts to "delay" other messages that may arrive while
flushing is in progress so that they will also be sent in the correct
order. It also delays the closing of the tunnel until pending data has been
flushed (unless an error occurs during the flush...).

Since the code is a bit complicated, I would appreciate several extra pairs
of eyes reviewing the code. My main areas of concern are (though please
feel free to add more):

* Does it solve the problem?
* Are there any race conditions/deadlocks that it might encounter ?
* Is throughput adversely impacted by the extra overhead involved in
processing incoming messages - even if no more pending messages.

The PR can be found at https://github.com/apache/mina-sshd/pull/72/ and a
branch containing this code can be checked out from
https://github.com/lgoldstein/mina-sshd/tree/SSHD-849.

I would really appreciate your feedback and review on the posted code.
Thanks
Lyor

Re: Please help review and test solution for SSHD-849

[Jonathan Valliere <jo...@apache.org>]

This is how I handle it,  Channel should go immediately into CLOSE_CLOSING
then into CLOSE_LINGER while flushing then CLOSE_CLOSED

On Fri, Oct 19, 2018 at 4:39 AM Emmanuel Lécharny <el...@gmail.com>
wrote:

> Hi Lyor,
>
> a few hints, onsidering I haven't reviewed the code...
>
> Le 19/10/2018 à 07:00, Lyor Goldstein a écrit :
> > Here is the issue in a nutshell - a client might open an SSH tunnel, send
> > some data and close (normally) its side of the tunnel before the channel
> to
> > the other side has been successfully established and all data
> transmitted.
> > Currently a race condition may occur in such a scenario where the code
> > closes the channel while pending messages are still in transit.
> >
> > The patch attempts to accumulate pending messages until channel is open
> > (successfully) and then flush them while maintaining their *order*. At
> the
> > same time, it attempts to "delay" other messages that may arrive while
> > flushing is in progress so that they will also be sent in the correct
> > order. It also delays the closing of the tunnel until pending data has
> been
> > flushed (unless an error occurs during the flush...).
>
> You should not have to deal with the delayed closing: MINA is already
> allowing you to do that, if you call closeOnFlush() instead of
> closeNow() -or close(), which maps to closeNow()-. It will then flush
> all the pending messages before closing the session. No message written
> in the session *after* you have called close(false) will be sent.
>
> At the same time, as soon as the closeOnFlush() method is called, we
> *should* stop reading anything from the client - ie set remove OP_READ
> flag from the selectionKey, something MINA is not doing (afaict). This
> is most certainly a missing part of the closeOnFlush(). The problem is
> that the OP_READ flag is registrered everytime we process the session,
> until the session is in CLOSING mode. That means we should probably set
> the session in CLOSING mode when we call closeOnFlush(), but we can't
> because this is a computed flag :/ So it boils down to check the session
> state - and we probably will need to add a additional flag like
> CLOSE_ON_FLUSH- when we try to update the event a channel is interested in.
>
> I would suggest filling a JIRA ticket for this one, it requires some
> further investigation, but I suspect it would ease yur implementation A
> LOT.
>
>
>
> --
> Emmanuel Lecharny
>
> Symas.com
> directory.apache.org
>
>

Re: Please help review and test solution for SSHD-849

[Emmanuel Lécharny <el...@gmail.com>]

Hi Lyor,

a few hints, onsidering I haven't reviewed the code...

Le 19/10/2018 à 07:00, Lyor Goldstein a écrit :
> Here is the issue in a nutshell - a client might open an SSH tunnel, send
> some data and close (normally) its side of the tunnel before the channel to
> the other side has been successfully established and all data transmitted.
> Currently a race condition may occur in such a scenario where the code
> closes the channel while pending messages are still in transit.
> 
> The patch attempts to accumulate pending messages until channel is open
> (successfully) and then flush them while maintaining their *order*. At the
> same time, it attempts to "delay" other messages that may arrive while
> flushing is in progress so that they will also be sent in the correct
> order. It also delays the closing of the tunnel until pending data has been
> flushed (unless an error occurs during the flush...).

You should not have to deal with the delayed closing: MINA is already
allowing you to do that, if you call closeOnFlush() instead of
closeNow() -or close(), which maps to closeNow()-. It will then flush
all the pending messages before closing the session. No message written
in the session *after* you have called close(false) will be sent.

At the same time, as soon as the closeOnFlush() method is called, we
*should* stop reading anything from the client - ie set remove OP_READ
flag from the selectionKey, something MINA is not doing (afaict). This
is most certainly a missing part of the closeOnFlush(). The problem is
that the OP_READ flag is registrered everytime we process the session,
until the session is in CLOSING mode. That means we should probably set
the session in CLOSING mode when we call closeOnFlush(), but we can't
because this is a computed flag :/ So it boils down to check the session
state - and we probably will need to add a additional flag like
CLOSE_ON_FLUSH- when we try to update the event a channel is interested in.

I would suggest filling a JIRA ticket for this one, it requires some
further investigation, but I suspect it would ease yur implementation A LOT.



-- 
Emmanuel Lecharny

Symas.com
directory.apache.org