The function MqttEndpoint.reject() close the NetSocketInternal before the client can receive the CONNACK

[mgsalafia]

I am using a browser version of mqtt.js to create an MQTT client and trying to connect with an MQTT broker created with vertx-mqtt.

When the broker reject the connection coming from the client from whatever reason, the client sometimes receive the CONNACK and sometimes don't (causing my mqtt.client to trying to reconnect indefinitely when a connack is not received).

From broker-side in my MqttEndpointHandler, i reject using the following code:

@Override
public void handle(MqttEndpoint endpoint) {
authService.verifyToken(token, edgeEndpoint)  //Returns a Future
                .onSuccess(user -> {
                   // Successful stuff...
                })
                .onFailure(throwable -> {
                    var error = WwmErrors.WwmError.fromJsonString(throwable.getMessage());
                    log.error("Error in verifyToken: {}", error);

                    //HERE I SEND A PROPER ERROR TO THE CLIENT USING endpoint.reject(...)
                    switch (error.errorCode()) {
                        case AUTH_API_USER_DATA:
                            endpoint.reject(MqttConnectReturnCode.CONNECTION_REFUSED_BAD_USERNAME_OR_PASSWORD);
                            break;
                        case AUTH_WWM_SERVICE_NOT_INCLUDED:
                            endpoint.reject(MqttConnectReturnCode.CONNECTION_REFUSED_NOT_AUTHORIZED);
                            break;
                        default:
                            endpoint.reject(MqttConnectReturnCode.CONNECTION_REFUSED_BAD_USERNAME_OR_PASSWORD);
                    }
                });
}

Going in depth, i found out that reject() in turn calls connack() of MqttEndpointImpl. The code of connack seems writing the message and closing the connection right after(this.close()). This seems to cause my issue because client-side the event 'close' on the websocket connection cause a 'reconnect' event and the connack is ignored.

Here the code of connack():

private MqttEndpointImpl connack(MqttConnectReturnCode returnCode, boolean sessionPresent, MqttProperties properties) {

        MqttFixedHeader fixedHeader = new MqttFixedHeader(MqttMessageType.CONNACK, false, MqttQoS.AT_MOST_ONCE, false, 0);
        MqttConnAckVariableHeader variableHeader = new MqttConnAckVariableHeader(returnCode, sessionPresent, properties);
        MqttMessage connack = MqttMessageFactory.newMessage(fixedHeader, variableHeader, (Object)null);
        
        this.write(connack); // HERE WRITE THE CONNACK.  <------ returns a Future that is ignored here
        if (returnCode != MqttConnectReturnCode.CONNECTION_ACCEPTED) {
            this.close(); // HERE CLOSE THE CONNECTION.      <------- This run without waiting for this.write() to complete
        } else {
            this.isConnected = true;
        }

        return this;
    }

As i commented in the code, this.write return a Future but is not being used at all and the method this.close() is called sequentially right after.

If i put a break point in this.close() (hence preventing the close of the websocket connection), the mqtt-client work like expected receiving the CONNACK for the mqtt connection and then printing the right error.

Is this a bug maybe? Or am i doing something wrong?

Version

4.2.1

[vietj]

I think it would be proper to close the connection after the future has been sent, can you try that and see what it gives ?

[msalafia]

@vietj i changed the connack() method as follows:

private MqttEndpointImpl connack(MqttConnectReturnCode returnCode, boolean sessionPresent, MqttProperties properties) {

    MqttFixedHeader fixedHeader =
      new MqttFixedHeader(MqttMessageType.CONNACK, false, MqttQoS.AT_MOST_ONCE, false, 0);
    MqttConnAckVariableHeader variableHeader =
      new MqttConnAckVariableHeader(returnCode, sessionPresent, properties);

    io.netty.handler.codec.mqtt.MqttMessage connack = MqttMessageFactory.newMessage(fixedHeader, variableHeader, null);

    // now close the socket connection or set IsConectd = true only when the future completes
    this.write(connack).onComplete(ar -> {
      // if a server sends a CONNACK packet containing a non zero return code it MUST then close the Network Connection (MQTT 3.1.1 spec)
      if (returnCode != MqttConnectReturnCode.CONNECTION_ACCEPTED) {
        this.close();
      } else {
        this.isConnected = true;
      }
    });

    return this;
  }

and i still have my issue. But, i think my problem is not due to this because, as far i understand just going depth in the code, the instruction this.write(connack) runs on the event loop, thus this.close() runs only when the write() completes (the next cycle of the event loop). So i think the problem is related to how the client-side websocket connection is managed in mqtt.js in the browser.

By the way, even though my problem is not due to the connack(), i think that the Future returned by the write() mehtod must be used in order to continue with either the close() call or the isConnected = true. The reason is that (i repeat, as far i can understand looking at the code) there are no guarantees that this.write() code runs in the event loop. In fact, going down following the write() call, there are lot of if statements checking if the channel context is in the event loop.

Hope this can help you.

[vietj]

do you know any way to reproduce this ?

[msalafia]

I think you can try to develop a simple javascript mqtt client using mqtt.js (in browser, not node.js) that just connect to the vertx backend. In the handle function of the handler just reject the connection. This is the simplest scenario i think.

[vietj]

@msalafia can you share a wireshark dump of the connection traffic ?

[vietj]

please provide a reproducer for this and reopen an issue