search

Azure / azure-iot-sdk-java

A Java SDK for connecting devices to Microsoft Azure IoT services
https://azure.github.io/azure-iot-sdk-java/
Other
200 stars 237 forks source link

[Device SDK / MQTT] DeviceClient closeNow not stopping threads #453

Closed jokarl closed 5 years ago

jokarl commented 5 years ago

Description of the issue:

Calling closeNow() leaves threads open. On startup, when connecting and calling openfor the first time:

Thread [MQTT Call: leafdevice] (Running)    
Thread [MQTT Rec: leafdevice] (Running) 
Thread [MQTT Snd: leafdevice] (Running) 
Thread [MQTT Disc: leafdevice] (Running)    
Thread [MQTT Snd: leafdevice] (Running) 
Thread [MQTT Rec: leafdevice] (Running) 
Thread [MQTT Call: leafdevice] (Running)    
Thread [MQTT Ping: leafdevice] (Running)

This result also seems weird, why are there duplicate Call, Rec and Snd threads? We are using a modified version of this sample to connect devices. We are only calling open once from our application code.

I leave the application running, but I call client.closeNow();and reconnect the device. The stack now looks like this:

Thread [MQTT Call: leafdevice] (Running)    
Thread [MQTT Rec: leafdevice] (Running) 
Thread [MQTT Snd: leafdevice] (Running) 
Thread [MQTT Disc: leafdevice] (Running)
Thread [MQTT Snd: leafdevice] (Running) 
Thread [MQTT Rec: leafdevice] (Running) 
Thread [MQTT Call: leafdevice] (Running)    
Thread [MQTT Disc: leafdevice] (Running)
Thread [MQTT Snd: leafdevice] (Running) 
Thread [MQTT Rec: leafdevice] (Running) 
Thread [MQTT Call: leafdevice] (Running)    
Thread [MQTT Ping: leafdevice] (Running)

Notice that there is one new instance of Call, Rec and Snd each, as well as a duplicate of Disc. Still only a single Ping thread.

Code sample exhibiting the issue:

141 discusses the same issue. It seems the related Paho issue has been resolved, but there have been no new releases of the SDK since this fix.

jokarl commented 5 years ago

I rebuilt iot-device-client locally with updated paho dependency:

<dependency>
    <groupId>org.eclipse.paho</groupId>
    <artifactId>org.eclipse.paho.client.mqttv3</artifactId>
    <version>1.2.1</version>
</dependency>

It now only creates 4 threads upon first calling open, calling closeNow and then open again and there are still 4 threads:

Thread [MQTT Rec: leafdevice] (Running) 
Thread [MQTT Snd: leafdevice] (Running) 
Thread [MQTT Call: leafdevice] (Running)    
Thread [MQTT Ping: leafdevice] (Running)

Would you like me to issue a pull request?

timtay-microsoft commented 5 years ago

I can create the PR for this. I'm glad to see Paho has fixed this issue as it has been around for a while. Thanks for bringing it to our attention.

jokarl commented 5 years ago

No problem, glad to help!

timtay-microsoft commented 5 years ago

Just to update this issue, we have attempted to upgrade our paho dependency to resolve this threading issue, but we are encountering regressions around mqtt_ws. We are still investigating the cause of these, and we'll try to keep this thread updated with our findings.

timtay-microsoft commented 5 years ago

Sorry for the intermittent updates, but to keep this thread up to date, Paho version 1.2.1 did introduce a regression on MQTT_WS connections, so until that issue is resolved, we cannot upgrade to fix this thread leak. I have opened an issue on Paho's issues page about this regression here.

jokarl commented 5 years ago

No worries, we've worked around it the meantime. I understand it's out of your control.

timtay-microsoft commented 5 years ago

A member of the MQTT Paho team is actively looking into the bug in Paho 1.2.1 that was preventing us from upgrading our dependency. We plan on updating and releasing our SDK as soon as that new Paho version becomes available. That new version of Paho should resolve this issue.

kartben commented 5 years ago

Just an update on this, the 1.2.2 release of Paho is imminent as per https://github.com/eclipse/paho.mqtt.java/issues/679#issuecomment-531194567

timtay-microsoft commented 5 years ago

We are working to upgrade our Paho dependency now that they have released version 1.2.2. Thankfully, it appears that this leaked thread bug is not present in 1.2.2 so once we upgrade our dependency, I will do a release of this SDK and close this issue.

timtay-microsoft commented 5 years ago

As of this release, the device client and deps will now use Paho version 1.2.2. This newest version of Paho fixed this issue, so I'm closing this issue.

az-iot-builder-01 commented 5 years ago

@hochas, @kartben, thank you for your contribution to our open-sourced project! Please help us improve by filling out this 2-minute customer satisfaction survey