zsmartsystems / com.zsmartsystems.zigbee

ZigBee Cluster Library Java framework supporting multiple dongles
Eclipse Public License 1.0
138 stars 87 forks source link

NPE in ZigBeeNetworkDiscoverer #1418

Closed amitjoy closed 4 months ago

amitjoy commented 4 months ago

The following exception occurred in our runtime and it seems that it should have never happened. I think it would be best to introduce the relevant preconditions to impose API's expectations.

I will try to open a PR for this.

java.lang.NullPointerException: null
    at com.zsmartsystems.zigbee.app.discovery.ZigBeeNetworkDiscoverer.getAssociatedNodes(getAssociatedNodes:388)
    at com.zsmartsystems.zigbee.app.discovery.ZigBeeNetworkDiscoverer.access$500(access$500:48)
    at com.zsmartsystems.zigbee.app.discovery.ZigBeeNetworkDiscoverer$3.run(run:324)
    at java.util.concurrent.Executors$RunnableAdapter.call(call:511)
    at java.util.concurrent.FutureTask.run(run:266)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(access$201:180)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(run:293)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(runWorker:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(run:624)
    at java.lang.Thread.run(run:750)
cdjackson commented 4 months ago

I suggest to do the same thing as we did before by adding this to the constructor. One other point, is that networkManager could be final (probably this is the same in the NCP PR you did previously although I didn't check).

However I'm not convinced this will resolve the issue. The error is apparently on this line -:

            CommandResult response = networkManager.sendTransaction(ieeeAddressRequest, ieeeAddressRequest).get();

I assume that networkManager is null, however if we look at where getAssociatedNodes is called, networkManager is used immediately before this - presumably without an NPE.

                    // If we don't know the node yet, then try to find the IEEE address
                    // before requesting the associated nodes.
                    if (networkManager.getNode(nodeNetworkAddress) == null) {
                        success = getIeeeAddress(nodeNetworkAddress);
                        return;
                    }

                    success = getAssociatedNodes(nodeNetworkAddress);

So presumably it was not null at that point and therefore any null check is likely to not help. This is similar to the previous issue you raised where it is "not possible" for this to happen, and I think without logs to provide som context, it's hard to say what is going on.

amitjoy commented 4 months ago

@cdjackson You are absolutely right that it will certainly not solve at all. We will require the logs to understand the situation more. Since I am missing the diagnostic data, we certainly cannot do anything about it. What we can at least do, is to ensure that the APIs enforce the invariants (preconditions) for consumer to know that somewhere downstream the consumer of the API (that throws NPE) is using NULL which is not intended.

You are right, about the code flow. It seems, networkManager.sendTransaction(ieeeAddressRequest, ieeeAddressRequest) returns null.

I will try to add the necessary preconditions to those places where we can enforce NPE and here, at least, we can provide an assertion for the transaction to not be null.