Hono: Hono client may become stalled while connecting because of stalled state in AMQP connection stack

[DanielMaier-BSI]

There are some states in AMQP connection stack used by hono where hono client ends up in a stalled state while connecting.

Examples are: AMQP Server does not respond to AMQP open frame, Server does not respond while SASL handshake (see also https://github.com/vert-x3/vertx-proton/issues/92) and maybe some others.

To protect hono client against such states, caused by bugs or missing features in used libraries, I would suggest to introduce some kind of overall connection timeout for connection attempts as a fallback. This timeout should abort the running connection attempt and trigger, if configured, a connection retry. This timeout needs not necessarily to be configurable, perhaps it is sufficient and avoids confusion if hono calculates an appropriate value from the other timeouts like TCP timeout etc.

WDYT?

[ctron]

I agree, the client side should always time out after it can't successfully connect. You never know what happens.

I am not so sure about the automatic retry, though. I think if it fails, then it fails. And the client (device) will re-trigger at some point in time anyways.

[calohmn]

Concerning the automatic retry:
I would treat a timeout waiting for a response during the SASL handshake the same way as when the server returns a SASL outcome indicating a transient error.
Such an error causes the reconnect handling in HonoClientImpl to retry opening the connection (if there's reconnectAttempts left).

[ctron]

Such an error causes the reconnect handling in HonoClientImpl to retry opening the connection (if there's reconnectAttempts left).

Ok, that is fine. I was assuming you wanted to add some additional re-connect handling.

[calohmn]

Concerning the configuration property for this new timeout feature:
We already have connectTimeout in ClientConfigProperties. This currently refers just to the time it takes for the TCP/TLS connection to be established.

Looking at the way the ProtonSaslClientAuthenticatorImpl class, handling the SASL handshake in the client, is hardwired to be used in vert.x-proton, it looks to me there's no good way to apply a timeout setting just for the SASL handshake.

Therefore I suggest changing the meaning of the existing connectTimeout property so that it refers to the time for TCP/TLS connection establishment _plus_ the time for the SASL handshake.

[sophokles73]

I agree that there (currently) is no good way to time out just the SASL handshake. I think it is generally ok to extend the scope of the connectTimeout property to also include the SASL handshake. However, I think we would then need to make sure that we ignore the reconnectAttempts set in the ProtonClientOptions and instead always only try to establish the TCP connection once and let HonoClientImpl's retry behavior kick in if that fails. Otherwise we might end up in situations where the full time period is used up by TCP reconnect attempts before the SASL handshake can even start. We could then probably remove the ProtonClientOptions parameter from the HonoClient methods altogether, because we wouldn't need/use any of its properties anymore anyway. WDYT?

[calohmn]

Good point about the ProtonClientOptions.reconnectAttempts. Currently it is set to 1 by default in ConnectionFactoryImpl. We could set it to 0 then.

We could then probably remove the ProtonClientOptions parameter from the HonoClient methods altogether

In so far as in Hono code there is only setConnectTimeout(DEFAULT_CONNECT_TIMEOUT_MILLIS) invoked on the ProtonClientOptions param, I agree.
However, there may be cases where this is still needed.
For example I've seen a use case outside of the Hono codebase where addEnabledSaslMechanism(ProtonSaslAnonymousImpl.MECH_NAME) is used in a unit test for the HonoClient.connect invocation.
Or other ProtonClientOptions settings might have their use cases as well.
Therefore I would keep the param - in the scope of this issue at least.

[sophokles73]

With https://github.com/eclipse/hono/commit/b6b00bbddc5a018687490ada7efc36a7de57e7eb the scope of the connection timeout has been extended to include

• the TCP/TLS connection,
• the SASL handshake (if necessary) and
• the reception of the peer's open frame