#66: Fix missing callback when TCP connect fails

[ctron]

When the TCP connect fails, then there is no notification via the
closeHandler. On the other side listening to the connect future for
errors notifies you twice of an error (via closeHandler and via future)
in cases where the TCP connect succeeded, but a CONACK with an error
is received.

This change adds a call to the closeHandler in case the TCP connect
fails, so consumers can solely rely on the closeHandler for failed
connection attempts and can re-trigger a connection, thus implementing
"auto reconnect".

Signed-off-by: Jens Reimann jreimann@redhat.com

[ppatierno]

I did't get it. Why using the connectHandler isn't enough for being notified by a TCP connection failure ?

[ctron]

The connectHandler isn't enough when the connection breaks. So you have the following scenarios:

TCP connect fails

• connectHandler gets fired with failed

TCP connect succeeds, CONACK is failed

• connectHandler gets fired with failed
• closeHandler gets fired

all succeeds, but fails later with TCP disconnect

• closeHandler gets fired

---

So now you are unable to handle failed -> reconnect in sane way. Because there is not clear path of notifying a connection failure. Especially in scenario 2, you would need to filter out one of the notifications. But you can't really do that, since you don't know if the other notification will get fired.

If however you would fire the close handler in scenario 1 as well, you could always handle connection failures via the path of closeHandler.

[ppatierno]

Can you point me to some code where you are trying the re-connect mechanism ? Can't you rely on the connectHandler provided to the connect method ? It's called when initial connection fails. Otherwise if an already established connection is closed, the closeHandler is raised.

[ctron]

Sure here is the code I am using, under the assumption this PR has been applied: https://github.com/ctron/flow/blob/develop/components/mqtt/src/main/java/de/dentrassi/flow/component/mqtt/MqttClient.java#L98

No I can't solely rely on the connectHandler as then I would not get notified when the connection breaks after the connection has been successfully established. The problem is with the scenario in the middle. When the TCP connection has been established, but the MQTT connection has not. In this case both handlers are fired.

[ppatierno]

@ctron imho the closeHandler makes sense when a connection is established and is working and then it's closed by the server or goes down for any other network reason. It doesn't make more sense on TCP connection establishment failure. I know that it sounds strange but maybe the reconnect feature should be developed inside the closeHandler; in the case when the TCP connection fails it should happen in the connectHandler. I guess that in this way you can cover both the scenarios without any changes to the MqttClient, right ?
@vietj what do you think ?

[vietj]

@ppatierno for close handler, I meant as complement to the built-in connection retry failure that NetClient does OOTB. It can either be externally done or built in the Mqtt client.

[vietj]

i.e in MqttClientImpl if internally done:

  void handleClosed() {
    synchronized (this.connection) {
      if (this.closeHandler != null) {
        this.closeHandler.handle(null);
      }
    }
  }

[ctron]

Ok, we just had a call … The problem is the scenario "TCP connect succeeds, CONACK is failed" where you get two callbacks with the meaning of "connection failed". @ppatierno will think of something 😉

[ppatierno]

Following the summary of the three possible scenarios :
a) TCP connection fails -> connectHandler is called
b) TCP ok -> traffic, traffic, ... -> server closes connection -> closeHandler is called
c) TCP ok -> server sends CONNACK != 0 -> both connectHandler and closeHandler are called (NOTE: closeHandler is called because due to MQTT spec, the server closes the established TCP connection)

So the @ctron fix works fine if the user ignores the connectHandler putting the logic for re-connection only inside the closeHandler. Of course, it's simpler but I still think that these handlers have different roles.

connectHandler is related to the initial connection establishment at TCP and then MQTT level (after checking the CONNACK response code) while closeHandler is related to the fact that the connection is closed later.

My opinion is that in any case for implementing manual re-connection the user should deal with both handlers because 1) user has to re-connect if the connection establishment process fails (TCP or CONNACK level) 2) user has to re-connect if the working connection is closed.

My idea is to add an isConnected status for the client (that could be exposed so useful to the user as well) which is set only when the connection establishment (CONNACK included) goes well. It can help to avoid calling the closeHandler in the case c).
It means no ambiguity but the user has to deal with both handlers for handling "manual re-connection".

That said, having auto-reconnect is a great feature to add but we need both because I can imagine users who don't want having auto-reconnect but using the manual one.

Just thinking aloud for having the best API for the user. @ctron @vietj wdyt ?

[ctron]

So would that mean that in the case 2./c) only the connectHandler gets fired?

[ppatierno]

@ctron with case 2/c) do you mean c) ? Yes only connectHandler gets fired.

[ctron]

Cool .. that would work fine!

[ctron]

Closing this out in favor of #71