Mqtt V5 improve automaticReconnect

[kroutal]

Expected Behavior
Mqtt V5 automaticReconnect should work when first connection fails.
This is related to : eclipse/paho.mqtt.java#930

Current Behavior
When application starts and server is down or something goes wrong during connection then In Mqttv5PahoMessageHandler/Mqttv5PahoMessageDrivenChannelAdapter there will never be a reconnection attempt even though automaticReconnect is set to true

Context

The only way to reconnect is to add a MqttConnectionFailedEvent listener. And recode the reconnection mechanism using Timers.
If this is the intended behavior then it should be documented.

[artembilan]

If such a behavior is not available in the Paho client, then I'm not sure what would you expect from us to here in our channel adapters.
And indeed the MqttConnectionFailedEvent you've mentioned should be a good workaround for time being.
This is the current logic in the Mqttv5PahoMessageHandler:

	protected void doStart() {
		try {
			this.mqttClient.connect(this.connectionOptions).waitForCompletion(getCompletionTimeout());
		}
		catch (MqttException ex) {
			ApplicationEventPublisher applicationEventPublisher = getApplicationEventPublisher();
			if (applicationEventPublisher != null) {
				applicationEventPublisher.publishEvent(new MqttConnectionFailedEvent(this, ex));
			}
			logger.error(ex, "MQTT client failed to connect. Will retry if 'ConnectionOptions.isAutomaticReconnect()'.");
		}
	}

so, if we fail to start/connect we emit that MqttConnectionFailedEvent.
And I believe it is explained well in docs:

MqttConnectionFailedEvent - published by both adapters if we fail to connect or a connection is subsequently lost.

Does it make sense?

[kroutal]

Thanks for your answer.

I was thinking of something similar to what is done in the MQTT V3 channel adapters, where a scheduler is started when connection fails on startup:

spring-integration/spring-integration-mqtt/src/main/java/org/springframework/integration/mqtt/inbound/MqttPahoMessageDrivenChannelAdapter.java

Line 183 in e855f13

scheduleReconnect();

Concerning the documentation, I was thinking of adding a disclaimer in this part of the documentation: https://docs.spring.io/spring-integration/docs/current/reference/html/mqtt.html#mqtt-v5.

	It is recommended to have the MqttConnectionOptions#setAutomaticReconnect(boolean) set to true to let an internal IMqttAsyncClient instance to handle reconnects. Otherwise, only the manual restart of these channel adapters can handle reconnects, e.g. via MqttConnectionFailedEvent handling on disconnection.

[artembilan]

Yeah... I see your point now having that opened issue in Paho client if it fails to connect originally, it does not retry.
So, you probably are right and we need to retry ourselves.
Perhaps similar way as we do with v3 channel adapters.
And do that independently of the automaticReconnect option.

Let us know if you are willing to contribute the fix: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc !

[kroutal]

I had a look at Paho Client implementation. The reconnect() will launch the scheduler. By calling this method once the behaviour will be as expected.
So an easier workaround could be

	protected void doStart() {
		try {
			this.mqttClient.connect(this.connectionOptions).waitForCompletion(getCompletionTimeout());
		}
		catch (MqttException ex) {
			if(this.connectionOptions.isAutomaticReconnect()){
                                //Add try catch
				this.mqttClient.reconnect();
			} else{
			ApplicationEventPublisher applicationEventPublisher = getApplicationEventPublisher();
			if (applicationEventPublisher != null) {
				applicationEventPublisher.publishEvent(new MqttConnectionFailedEvent(this, ex));
			}
			logger.error(ex, "MQTT client failed to connect. Will retry if 'ConnectionOptions.isAutomaticReconnect()'.");
                      }
		}
	}

I'm willing to contribute

[artembilan]

Thanks, @kroutal !

Indeed the Paho Client does not move the connection to the proper state on a first attempt.
I see that they start ClientComms.ConnectBG which has a logic like:

		if (wasConnected && callback != null) {
			// Let the user know client has disconnected either normally or abnormally
			callback.connectionLost(reason, message);
		}

to call the mentioned reconnectTimer.schedule(new ReconnectTask(), reconnectDelay); background task.
But the problem that this wasConnected is not true on a first attempt:

wasConnected = (isConnected() || isDisconnecting());

It was not connected before and it is really not disconnecting at the moment.
The state is just CONNECTING 🤷

So, according to your investigation it is really correct to call the mentioned reconnect() from our doStart().
I think we need to check for this.connectionOptions.isAutomaticReconnect() when this reconnect() fails and only after that emit the event.
If it is automatic, it seems that the task is going to be scheduled and no error are thrown back to end-user.

So, it really would be great if you can contribute the fix and we will continue discuss details already in code.