change isConnected flag only in handleClose

[Sammers21]

Partly solves #109

[Sammers21]

@vietj , @ppatierno , can you review, please?

[ffeii]

@Sammers21 , the isConnected field will never be true.

[vietj]

I think we need to have a test for the issue you are trying to fix

[ppatierno]

@Sammers21 with this change, the client is going to close the TCP connection if it gets a negative CONNACK from the server, right? Why it should do that? On the same TCP connection the client could be able to send a new CONNECT packet to be successful (maybe the problem is about authentication with username and password). Tbh I don't remember why the close handler was tight to the isConnected flag so that we can just remove the check on isConnected in the handleClosed.

[Sammers21]

On the same TCP connection the client could be able to send a new CONNECT packet to be successful (maybe the problem is about authentication with username and password)

Yes, but the current API does not allow as to do this, I mean sending COONECT twice on the same connection.

Tbh I don't remember why the close handler was tight to the isConnected flag so that we can just remove the check on isConnected in the handleClosed.

I think we need to clearly define what isConnected and handleClose means. Either they are related to a connection on the TCP level or MQTT session.

[SunFeiyue]

Tbh I don't remember why the close handler was tight to the isConnected flag so that we can just remove the check on isConnected in the handleClosed.

@ppatierno @Sammers21 Or, is it possible to add a new flag to indicate the TCP connection is connected?

[vietj]

I think having first a test that reproduce the issue would be good, so we can then settle or not on the usefulness of the PR.

[ppatierno]

I agree with Julien, we first need a test for reproducing it and then we can think about the better solution.

[ffeii]

public class ConnectHandlerNotCalled extends io.vertx.core.AbstractVerticle {

    public static void main(String... args) {
        io.vertx.core.Vertx.vertx().deployVerticle(ConnectHandlerNotCalled.class.getName());
    }

    @Override
    public void start() throws Exception {

        io.vertx.mqtt.MqttServer.create(vertx).endpointHandler(endpoint -> {
            System.out.println("CONNECT from " + endpoint.clientIdentifier());
            // Do not send CONNACK
            // endpoint.accept(false);
        }).listen(1883, lh -> {
            System.out.println("Server is listening");
            io.vertx.mqtt.MqttClient.create(vertx).connect(1883, "127.0.0.1", ch -> System.out.println("ConnectHandler called"));
        });
    }
}

[Sammers21]

Just added a test case. If we don't receive a CONNACK packet within a period of time, we will treat it as an unsuccessful attempt to establish a MQTT session

[konradmichael]

I also recommend to change the title, if possible.
The isConnected variable is still changed in the conAck-handler

[konradmichael]

I do not see a reason to set a timer that closes the endpoint.
It is sufficient to do nothing to make the test pass.

Keeping this action could lead to the conclusion that your waiting for the TCP-Connection to be closed, which is not what is intended