mqtt client will never realize the connection is closed when TCP connection established but CONACK not received

[SunFeiyue]

For the following situation :

1. the underlying TCP connection is established
2. client receives no CONACK
3. the underlying TCP connection is disconnected

In this situation, the mqtt client "connect handler" will not be called because CONACK is not received, the mqtt client 'close handler' will not be called because 'isConnected' is false. So the client will never realize the connect result because it doesn't receive any feedback, except monitoring the 'isConnected' value.

[vietj]

why does it matter since that's not a real connection ?

[SunFeiyue]

why does it matter since that's not a real connection ?

Since the client dosen't receive any feedback, the client code will not be triggered.
We have met such a problem: we have implemented auto-connect function when the mqtt connection is broken. We wrote the 'connect-retry' logic in the 'close handler' and 'connect handler'. In 'connect handler', we check that if the connect fails, we will do 'connect-retry' after a period of time. In 'close handler', we will also do 'connect-retry' after a period of time. In the above situation, since the connect handler and close handler will not be called, our 'auto-connect' mechanism will fail.

[ppatierno]

@SunFeiyue I think that it makes sense. The close handler is supposed to be called when the TCP connection is closed and should not be tied to the CONNACK. Thanks for raising it.
Are you going to provide a PR for this one?

[vietj]

@ppatierno ok, so that would mean that the client sends at least a CONNECT message right ?

[vietj]

I thought the client would not send anything first :-)

[ppatierno]

@vietj the client should do that: opening the TCP connection, sending the CONNECT and waiting for the CONNACK. I think that the problem they are facing is about the CONNECT cannot reach the server (so it doesn't send CONNACK) or the client doesn't receive a CONNACK and somehow the server closes the TCP connection (or it's interrupted). Is that right @SunFeiyue ?

[vietj]

if CONNECT does not reach the server then it will not help because the handler won't get MqttEndpoint and won't have opportunity to set a close handler

[ppatierno]

Yeah agree but @SunFeiyue is talking about "the underlying TCP connection is disconnected" so it should be detected at Netty "level" on the client and the close handler should be called. Of course, if the TCP connection is not disconnected, the client could wait indefinitely because it can be related to the CONNECT or CONNACK lost but with the TCP connection still in place.

[Sammers21]

I think we can have a readTimeout option, which can be applied not only for the MQTT connection phase(CONNECT & CONNACK).

[SunFeiyue]

@vietj the client should do that: opening the TCP connection, sending the CONNECT and waiting for the CONNACK. I think that the problem they are facing is about the CONNECT cannot reach the server (so it doesn't send CONNACK) or the client doesn't receive a CONNACK and somehow the server closes the TCP connection (or it's interrupted). Is that right @SunFeiyue ?

Yes, right. Sorry for later reply.

[SunFeiyue]

I think we can have a readTimeout option, which can be applied not only for the MQTT connection phase(CONNECT & CONNACK).

I don't think this fix can solve my problem actually. In the situation I mentioned above, io.vertx.mqtt.impl.MqttClientImpl#handleClosed will be called when the TCP connection become disconnected, but the close handler will not be called, just because the 'isConnected' field is false.

void handleClosed() {
    synchronized (this.connection) {
      boolean isConnected = this.isConnected;
      this.cleanup();

      if (this.closeHandler != null && isConnected) {
        this.closeHandler.handle(null);
      }
    }
  }

[Sammers21]

@SunFeiyue , the source code for the io.vertx.mqtt.impl.MqttClientImpl#handleClosed is following:

void handleClosed() {
   synchronized (this) {
     boolean isConnected = this.isConnected;
     this.isConnected = false;
     if (!isConnected) {
       return;
     }
   }
   Handler<Void> handler = closeHandler();
   if (handler != null) {
     handler.handle(null);
   }
 }

[Sammers21]

Sorry, it was a miss click(closing this issue). PR is also reverted

[SunFeiyue]

@SunFeiyue , the source code for the io.vertx.mqtt.impl.MqttClientImpl#handleClosed is following:

yes, it is. I have seen this lastest source code. In the lastest handleClosed method, the close handler will still not be called when isConnected is false.

[Sammers21]

@SunFeiyue, I agree. I think the isConnected field should be changed only in the handleClose method.