com.zsmartsystems.zigbee icon indicating copy to clipboard operation
com.zsmartsystems.zigbee copied to clipboard

NPE in ZigBeeNetworkDiscoverer

Open amitjoy opened this issue 1 year ago • 1 comments

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)

amitjoy avatar Feb 21 '24 17:02 amitjoy

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.

cdjackson avatar Feb 21 '24 19:02 cdjackson

@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.

amitjoy avatar Feb 22 '24 08:02 amitjoy