Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
[paho-dev] Wrong Design Pattern in ClientState.checkForActivity()

Hi everyone,

I was using Paho-java on Android and I found that sometimes the callback set in MqttToken to be executed when Ping completes is not called. Since the Ping sender will hold a WAKE_LOCK which will only be released using the callback, in this case, the WAKE_LOCK will never be released, causing the Android to stay awake and drain battery.

To reproduce this bug, simply add a line of
 Thread.sleep(1000);
in AlarmPingSender.AlarmReceiver.onReceive() function in org.eclipse.paho.android.service, after this line:
comms.checkForActivity();
to simulate the circumstances that the network thread runs faster than the "control thread", which is the thread running onReceive() function.

Current implementation assumes that the "control thread" is faster than the network thread. However,  this is not necessarily true and more over, I see no lock mechanism to ensure this assumption stands.

Here is my fix for now. I changed the signature of ClientState.checkForActivity() to accept a IMqttActionListener object and attach the callback to MqttToken before it is inserted into the event queue. Now the callback can work without the assumption that "controller thread" runs faster than "network thread". I also added some code to ensure the callback is called when a network exception is thrown or the client disconnect actively.

I haven't read through Paho's source code but I'm worried that if such bugs also exist in other parts of Paho since this bug is caused not by a single mistake, but a wrong design pattern. I'm wondering if some kind of code review will be carried out to see if there are more bugs of this kind.

Best regards,

Bob

Back to the top