Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incomplete shutdown of Mqttv5PahoMessageDrivenChannelAdapter #3697

Closed
mrpiggi opened this issue Dec 16, 2021 · 1 comment · Fixed by #3698
Closed

Incomplete shutdown of Mqttv5PahoMessageDrivenChannelAdapter #3697

mrpiggi opened this issue Dec 16, 2021 · 1 comment · Fixed by #3698

Comments

@mrpiggi
Copy link
Contributor

mrpiggi commented Dec 16, 2021

In what version(s) of Spring Integration are you seeing this issue?

5.5.6

Describe the bug

Calling Mqttv5PahoMessageDrivenChannelAdapter.doStop() does not disconnect and close the underlying IMqttAsyncClient

@Override
protected void doStop() {
this.topicLock.lock();
String[] topics = getTopic();
try {
this.mqttClient.unsubscribe(topics).waitForCompletion(getCompletionTimeout());
}
catch (MqttException ex) {
logger.error(ex, () -> "Error unsubscribing from " + Arrays.toString(topics));
}
finally {
this.topicLock.unlock();
}
}

Expected behavior

I am not sure if the current behavior is intended or a bug. At least, calling MqttPahoMessageDrivenChannelAdapter.doStop() does exactly that

try {
this.client.disconnectForcibly(this.disconnectCompletionTimeout);
}
catch (MqttException ex) {
logger.error(ex, "Exception while disconnecting");
}
this.client.setCallback(null);
try {
this.client.close();
}
catch (MqttException ex) {
logger.error(ex, "Exception while closing");
}

If the current behavior is as desired, what do I need to do to disconnect and close the underlying IMqttAsyncClient?

@mrpiggi mrpiggi added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Dec 16, 2021
@artembilan
Copy link
Member

I think we are missing this.mqttClient.disconnect().waitForCompletion(getDisconnectCompletionTimeout()); alongside with that unsubscribe().
The client.close() has to be done in the destroy() implementation, which we also are missing at the moment.

Thank you! Will fix shortly.

@artembilan artembilan added this to the 5.5.7 milestone Dec 16, 2021
@artembilan artembilan added in: mqtt and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Dec 16, 2021
artembilan added a commit to artembilan/spring-integration that referenced this issue Dec 16, 2021
Fixes spring-projects#3697
SO: https://stackoverflow.com/questions/70374046/spring-integration-mqtt-failed-to-start-app-when-the-network-is-disconnected

* Add `mqttClient.disconnect()` to `Mqttv5PahoMessageDrivenChannelAdapter.doStop()` - the `doStart()` does `connect()`
* Add `Mqttv5PahoMessageDrivenChannelAdapter.destroy()` impl to close `mqttClient`
* Fix `Mqttv5PahoMessageHandler.doStart()` to not re-throw an exception on connection.
Emit an `MqttConnectionFailedEvent` and log error instead
* Fix `Mqttv5PahoMessageHandler.destroy()` to call `mqttClient.close(true)` for better resources clean up
* Improve MQTT v5 components Javadocs and add a reconnect note into `mqtt.adoc`
garyrussell pushed a commit that referenced this issue Dec 16, 2021
Fixes #3697
SO: https://stackoverflow.com/questions/70374046/spring-integration-mqtt-failed-to-start-app-when-the-network-is-disconnected

* Add `mqttClient.disconnect()` to `Mqttv5PahoMessageDrivenChannelAdapter.doStop()` - the `doStart()` does `connect()`
* Add `Mqttv5PahoMessageDrivenChannelAdapter.destroy()` impl to close `mqttClient`
* Fix `Mqttv5PahoMessageHandler.doStart()` to not re-throw an exception on connection.
Emit an `MqttConnectionFailedEvent` and log error instead
* Fix `Mqttv5PahoMessageHandler.destroy()` to call `mqttClient.close(true)` for better resources clean up
* Improve MQTT v5 components Javadocs and add a reconnect note into `mqtt.adoc`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants