Bug #273

Unregister consumers correctly in case of failed communication

Added by Philipp Buluschek almost 4 years ago. Updated over 3 years ago.

Status:Closed Start date:11/10/2014
Priority:Normal Due date:
Assignee:Stefano Lenzi % Done:

0%

Category:zigbee.basedriver Spent time: -
Target version:org.aaloa.zb4osgi.zigbee.basedriver-0.8.0
Has a patch:No Has license agreement signed:No

Description

In ZigBeeDeviceImpl.invoke(Cluster), a new WaitForClusterResponse registers itself as a consumer in the ZigBeeDeviceImpl to wait for the async response from the device.

If the communication fails (result null, SRSP.status !=0 etc), the WaitForClusterResponse is not being deregistered. It will deregister itself on the next async message.

This creates two problems:
- without transaction IDs, the wrong consumer might consume an async message which comes in later and which is not really intended for it (likely causes bug #237).
- with transaction IDs, the consumers keep being registered and each time a failed write happens, they stack up, creating a memory leak.

Also, registering as a listener in the constructor (in WaitForClusterResponse) is not recommended as it leaks the this reference while the object is not fully constructed. (see eg. http://stackoverflow.com/questions/3921616 ).

I would recommend doing the registration explicitely in ZigBeeDeviceImpl:

public Cluster invoke(Cluster input) throws ZigBeeBasedriverException {
    ...
    WaitForClusterResponse waiter = new WaitForClusterResponse(
        this, transaction, input.getId(), TIMEOUT); // remove self registration as listener in constructor for this!

    m_addAFMessageListener(); // must be done before registering the consumer
    addAFMessageConsumer(waiter);
    ...

    if( response == null){
        removeAFMessageConsumer(waiter); 
        m_removeAFMessageListener(); // must be done after removing consumer
        throw new ZigBeeBasedriverException(logHead + "Unable to send cluster on the ZigBee network due to general error - is the device sleeping?");
    ...

Associated revisions

Revision 1092
Added by Stefano Lenzi almost 4 years ago

Created JUnit for verifying stale WaitForClusterResponse reference ( refs #273 )

Revision 1093
Added by Stefano Lenzi almost 4 years ago

WaitForClusterResponse does not use anymore inversion of control and it is more readable adding and removing it as listener
Avoiding stale reference for WaitForClusterResponse ( refs #273 )

History

#1 Updated by Stefano Lenzi almost 4 years ago

  • Category set to zigbee.basedriver
  • Status changed from New to In Progress
  • Assignee set to Stefano Lenzi
  • Priority changed from High to Normal
  • Target version set to org.aaloa.zb4osgi.zigbee.basedriver-1.X

Philipp I confirm the bug and it is quite importante because on the long run it will cause a memory leak

Regarding the this reference, it is not a big deal to have the reference, but taking it out will improve readability of the code

#2 Updated by Stefano Lenzi almost 4 years ago

  • Status changed from In Progress to Resolved

I think it is now fixed, please close it if you agree

I have also removed the this reference for improving readability of the code, now WaitForClusterResponse can be used only for a single response, but it was used like that even before

#3 Updated by Philipp Buluschek almost 4 years ago

  • Status changed from Resolved to Closed

Looks good.

#4 Updated by Stefano Lenzi over 3 years ago

  • Target version changed from org.aaloa.zb4osgi.zigbee.basedriver-1.X to org.aaloa.zb4osgi.zigbee.basedriver-0.8.0

Also available in: Atom PDF