From 9bd52d7a8f59c3438a88587eab32409d36bba552 Mon Sep 17 00:00:00 2001 From: Morten Mey Date: Tue, 4 Nov 2025 10:24:47 +0100 Subject: [PATCH] Fix race condition between disconnect and close If close happened immediately after disconnect the following happened: 1. disconnect sets comm state to DISCONNECTING and starts a new thread to actually do the disconnect. 2. close sees this state and only sets closePending to true. 3. The new thread from 1 does some cleanup and sends the MQTT disconnect message and waits for it to be sent. 4. The disconnect thread calls shutdownConnection(...) which simply returns and does nothing if closePending is true. So if 2 happens before 4 no one executes the shutdownConnection logic and the client is left in a unusable but also not cleaned up state. Triggering this race condition is extremly easy if someone is just trying to properly cleanup a connected client with the following code: client.disconnect(); client.close(); Simply removing the check for closePending in the start of shutdownConnection fixes this. Removing this check should also not introduce new issues, since the close() itself does not cause a call to shutdownConnection. Which means that if there is another race condition here then such a condition can already be triggered by other means as well. There is a small gap where in theory the user code could attempt to create a new connection, but that will fail, since the connect call also checks the closePending flag. Closes: #1093 Signed-off-by: Morten Mey --- .../org/eclipse/paho/client/mqttv3/internal/ClientComms.java | 2 +- .../org/eclipse/paho/mqttv5/client/internal/ClientComms.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/org.eclipse.paho.client.mqttv3/src/main/java-templates/org/eclipse/paho/client/mqttv3/internal/ClientComms.java b/org.eclipse.paho.client.mqttv3/src/main/java-templates/org/eclipse/paho/client/mqttv3/internal/ClientComms.java index b0c433498..c219569e5 100644 --- a/org.eclipse.paho.client.mqttv3/src/main/java-templates/org/eclipse/paho/client/mqttv3/internal/ClientComms.java +++ b/org.eclipse.paho.client.mqttv3/src/main/java-templates/org/eclipse/paho/client/mqttv3/internal/ClientComms.java @@ -351,7 +351,7 @@ public void shutdownConnection(MqttToken token, MqttException reason) { // This method could concurrently be invoked from many places only allow it // to run once. synchronized(conLock) { - if (stoppingComms || closePending || isClosed()) { + if (stoppingComms || isClosed()) { return; } stoppingComms = true; diff --git a/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/internal/ClientComms.java b/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/internal/ClientComms.java index 7a8463242..5cfba91e7 100644 --- a/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/internal/ClientComms.java +++ b/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/internal/ClientComms.java @@ -391,7 +391,7 @@ public void shutdownConnection(MqttToken token, MqttException reason, MqttDiscon // This method could concurrently be invoked from many places only allow it // to run once. synchronized (conLock) { - if (stoppingComms || closePending || isClosed()) { + if (stoppingComms || isClosed()) { return; } stoppingComms = true;