SSHD-968 interpret SSH_MSG_UNIMPLEMENTED response to a global request as its failure

[lgoldstein]

The main flow is as follows:

• when AbstractSession#preProcessEncodeBuffer override is called as part of sending the global request, it checks if the buffer about to be encoded and written is a SSH_MSG_GLOBAL_REQUEST command. If so, it stores the outgoing message sequence number in case SSH_MSG_UNIMPLEMENTED is reported for it.

• AbstractSession#request resets the tracked global request outgoing sequence number if either a response was received or timeout expired.

• AbstractSession#doInvokeUnimplementedMessageHandler override checks if the reported message number matches the currently tracked global request message and converts it to a message failure signal instead (thus releasing the AbstractSession#request loop waiting for it).

[gnodet]

I thought you wanted a less invasive fix ;-)
I don't really like the artificial split with the introduction of the preProcessEncodeBuffer method as it's only called from AbstractSession. encode, while the preProcessEncodeBuffer is defined in the SessionHelper and overriden in the AbstractSession. This looks rather complex instead of simply adding the few needed lines in the existing encode method (efdb7c3#diff-916a03c44b5846431681a4411891d952R843-R849).
Anyway, if you really wanted something simple, we could go for gnodet@cdefec7 which is much simpler ;-)

[lgoldstein]

I thought you wanted a less invasive fix ;-)

I did not mean less invasive as much as less aware of the purpose of the changes. It is less invasive as far as existing methods since it only introduces new ones.

I don't really like the artificial split with the introduction of the preProcessEncodeBuffer method as it's only called from AbstractSession. encode, while the preProcessEncodeBuffer is defined in the SessionHelper and overriden in the AbstractSession. This looks rather complex instead of simply adding the few needed lines in the existing encode method (efdb7c3#diff-916a03c44b5846431681a4411891d952R843-R849).

True, but it was the only way that I could come up with - IMO encode already does so much (lots of code for one method) so I thought it would make sense to split it rather than add to it. Anyway, like I said it opens up the door for future fixes/extension - e.g., perhaps one of the derived classes from AbsractSession needs something else.

Anyway, if you really wanted something simple, we could go for gnodet/mina-sshd@cdefec7 which is much simpler ;-)

True, but IMO this fix while more complex feels more robust. Any objections to merging it ?

[gnodet]

No objections, go for it.