a timeToWait value of -1 (i.e. wait forever) is not a good default

[cuddlyporcupine]

Please fill out the form below before submitting, thank you!

• [x] Bug exists Release Version 1.2.2 ( Master Branch)
• [ ] Bug exists in MQTTv3 Client on Snapshot Version 1.2.3-SNAPSHOT (Develop Branch)
• [ ] Bug exists in MQTTv5 Client on Snapshot Version 1.2.3-SNAPSHOT (Develop Branch)

If this is a bug regarding the Android Service, please raise the bug here instead: https://github.com/eclipse/paho.mqtt.android/issues/new

The default time to wait value: -1 . Is not a good default value. This occasionally leads clients to hang in the call to connect( ) indefinitely. 60000 milliseconds is a reasonable default and will avoid some "my connection is hung" bugs in the future. It won't affect users that are already calling setTimeToWait API, but there are quite a few applications that don't call the setTimeToWait API.

Code reference: https://github.com/eclipse/paho.mqtt.java/blob/master/org.eclipse.paho.client.mqttv3/src/main/java/org/eclipse/paho/client/mqttv3/MqttClient.java#L64

Recommendation:

• Change from -1 to 60000

[rdasgupt]

@cuddlyporcupine I think, changing timeToWait from -1 to 60000 for a Synchronous client is not correct. The default behavior of a Synchronous client should be blocking. User can always change this behavior by setting timeToWait using setTimeToWait() method. Also changing the default value will make all existing clients in the field, behave differently and may result in questions/defects.

This is a good recommendation though and should be documented as a best practice.

@icraggs Any comment?

[ThomasStubbe]

I also agree with cuddlyporcupine. Waiting infinite is not a good solution! Not for a asynchronous client, not for a synchronous client. BTW, whether this code is executed synchronous or asynchronous depends on your implementation. With the default setting in a asynchronous flow, a memory leak is created. In a synchronous flow, your application hangs. What is wrong with throwing a TimeoutException? (not a RuntimeException!). The call is and stays blocking, no matter the time-out value.

We just had this issue in a production setup to IoT Core from AWS, it only started happening since QOS 1 was introduced. A scheduled task periodically sent a message, but this got stuck infinitely without logs or anything.

But even more worrisome, is that when setting timeToWait, that "In the event of a timeout the action carries on running in the background until it completes." according to the java-docs. So this creates a memory leak on purpose. What is the reason for this strange behavior?

[ThomasStubbe]

I have an extra question for this. In my opinion this timeToWait is actually not a time-out. It only makes sense to use 1 or -1, where the 1 means it's an async call, while the -1 means that it's sync. This should be a boolean. How can we implement an actual timeout, where the execution stops? Or is it expected to write your own threads and manage/kill them to prevent memory leaks?

[NathanSweet]

The timeToWait setting is not good for asynchronous behavior. If I want to call a method and continue on immediately, I have to set timeToWait to 1 and catch the exception. This is clunky, plus the wait of 1ms. MqttAsyncClient should be used instead.

The timeToWait setting is only good for a response timeout. Ie, if a response is not received within the timeout then it's catastrophic.