Re: Please help review and test solution for SSHD-849
>> 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 flushall the pending messages before closing the session. No message written in the session *after* you have called close(false) will be sent. For all I know you may be right (I have not reviewed this option in depth) - please note though that the SSHD code does not deal directly with MINA but rather uses an *abstraction layer* that enables it to invoke various different network transports - MINA being one of them (NIO2 and Netty being others - and in the future... who knows). That being said, we try to keep the transport implementation requirements rather simple so others may find it easier to implement a new transport. Requiring a specialized feature such as this one (or the other you mention) is likely to complicate future implementations. Furthermore, even if we made such features optional (e.g., via *isFeatureXXXSupportedI) *both the SSHD code and the compatibility matrix (which feature is required for which SSHD functionality) would make it (IMO) complex (more than already is) and cumbersome to use (users would have to be aware what are they *giving up* when switching transports. E,g, what if users need 2 features - one of which is supported by one transport but not the other and the other feature only by its counterpart. >> I would suggest filling a JIRA ticket for this one, it requires some further investigation, but I suspect it would ease your implementation A LOT Again, you may be 100% right, but in view of the above I do not want to request lots of work (perhaps complex) on what may prove to be a "niche" piece of code especially tailored for SSHD. Furthermore, I am not sure this is exactly what is needed in this case - and until I am reasonably sure I can define the exact requirements I don't think it would be a good idea. Thanks for the feedback, Lyor
Re: Please help review and test solution for SSHD-849
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 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
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 pEpkey.asc Description: application/pgp-keys
