[
https://issues.apache.org/jira/browse/SSHD-1295?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17602860#comment-17602860
]
Thomas Wolf commented on SSHD-1295:
-----------------------------------
PR 242 works now. It looks OK to me, but I'm not sure about it's the right way.
I'd appreciate your opinions, [~lgoldstein] and [~gnodet].
I'm not sure it fully resolves this problem. It certainly works if there is
only one thread calling {{{}ConnectFuture.verify(){}}}.
What is the programming model and contract here anyway? It _is_ theoretically
possible that several threads would call {{verify(timeout)}} on the same
{{ConnectFuture}} object, possibly even with different time-outs. The current
code in PR 242 will abort the connection (or rather, close the established
connection and session) if _any_ of them times out before the {{ClientSession}}
is established (set on the future).
Another programming model would be to abort the connection only if the future
is _canceled_ and close the session only on cancellation. That would leave it
up to the application to decide under which circumstances it wants to treat a
time-out as a connection abort, and if so call {{cancel()}} explicitly, but it
will make using the {{ConnectFuture}} correctly even in the
single-thread-calls-verify case much more complicated.
Alternatively we could specify the contract that {{verify()}} must not be
called by two threads concurrently. If so, we might even enforce that contract.
I also see that cancellations are only propagated upwards (MINA or Netty future
-> {{IoConnectFuture}} -> {{{}ConnectFuture{}}}, or in the NIO2 completion
listener {{IoConnectFuture}} -> {{{}ConnectFuture{}}}, but in the latter case I
see no place where {{IoConnectFuture.cancel()}} is called at all, and in the
MINA/Netty cases, I don't know where the MINA or Netty future might be
cancelled – perhaps inside the MINA/Netty libraries). I see no downward
propagations (Application calls {{ConnectFuture.cancel()}} ->
{{IoConnectFuture}} is not cancelled). So I suspect we could get the same
established connection and session behind the scenes on cancellations. PR 242
does not handle that case. Should it?
Another shortcoming of the current approach in PR 242 is that it does create
the {{ClientSession}} first, which already sends messages, and only then
decides to close the session again. However, avoiding that would be a much more
invasive change that would have to re-define the way a {{ClientSession}} is
started: basically, don't send messages in the constructor already but have a
separate {{start()}} method that would have to be called after the session
object was fully created. I would suggest that any attempts to fix that should
be done in separate changes. First let's get this right on
{{{}ClientSession{}}}/{{{}ConnectFuture{}}} level and think about the
{{{}IoSession{}}}/{{{}IoConnectFuture{}}} level and the NIO2/MINA/Netty levels
later on.
> Connection attempt not canceled when a connection timeout occurs
> ----------------------------------------------------------------
>
> Key: SSHD-1295
> URL: https://issues.apache.org/jira/browse/SSHD-1295
> Project: MINA SSHD
> Issue Type: Bug
> Affects Versions: 2.9.2
> Reporter: Thomas Wolf
> Priority: Major
>
> This is not a new bug; it's also present in earlier versions.
> When {{client.connect().verify(timeout)}} times out, the underlying
> asynchronous connection attempt still continues _and may still succeed_. If
> that happens, there is a connection and session that the user cannot use or
> close. (Short of calling verify().getSession() again, but that is a pattern I
> have never seen, and it also makes using a timeout rather useless.)
> This means that timeouts are inherently unreliable.
> Here's a simple test case illustrating this problem (for
> {{org.apache.sshd.client.ClientTest}}):
> {code:java}
> @Test
> public void testConnectTimeout() throws Exception {
> client.start();
> try {
> ConnectFuture future = client.connect(getCurrentTestName(),
> TEST_LOCALHOST, port);
> try {
> future.verify(1);
> fail("Timeout expected");
> } catch (InterruptedIOException | SshException e) {
> ClientSession session = null;
> try {
> session = future.verify(CONNECT_TIMEOUT).getSession();
> } catch (SshException e2) {
> assertTrue("Expected a timeout, got " + e2,
> e2.getMessage().contains("timeout"));
> }
> if (session != null) {
> assertTrue("Session should be closed", session.isClosed() ||
> session.isClosing());
> }
> }
> } finally {
> client.stop();
> }
> }
> {code}
> This test fails.
> When the {{ConnectFuture}} times out (or is canceled, or waiting is
> interrupted), the underlying {{IoConnectFuture}} must be canceled. If we do
> get an {{IoSession}} all the same, but the {{ConnectFuture}} is already
> canceled or timed out, the {{IoSession}} must be closed immediately.
> Preferrably before a {{Session}} is created at all; otherwise, that SSH
> session must be closed, too. Creating a {{ClientSession}} already starts the
> application-level protocol (sending the SSH ident, or if a proxy is used, the
> proxy connect request).
> The test also highlights another problem: there is no clear indication that
> the failure of {{verify()}} indicates a timeout. Probably there should be a
> dedicated exception for this, or at least the {{SshException}} should have a
> {{TimeoutException}} as cause.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
