Closed TomTravaglino closed 2 years ago
Hey @TomTravaglino - happy 2022 :) I hope things are going well for you.
I'm generally happy with this and see the issue you're trying to resolve. I just wonder if there's some optimisation we can make. Currently, we have a lot of checks if (frameHandler == null)
which are effectively there to do exactly the same thing as the isShutdown
flag you have added, but of course frameHandler = null
is set a little later in the shutdown sequence, so it leaves the window open for the executor issue you note here.
I wonder if we shouldn't change the if (frameHandler == null)
checks to if (!isShutdown)
. In many cases it doesn't matter too much, but in the sendCommand
methods we now have both checks and this is really a bit redundant I think. We could just make the change here, but I think for consistency if we add a isShutdown
flag, we might as well use it consistently.
My only slight reservation with this is I'm trying to see if it's possible that frameHandler
can be null anywhere during initialisation that a isShutdown
check would not catch given isShutdown
is set to false. A further option therefore could be to change the name of your flag to isConfigured
, initialise it to false
, and set to true
during initialisation, and false
in shutdown
.
What do you think? It just seems a little tidier to me even though I appreciate it's really only solving the sequential if
statements in the sendCommand
method :)
@cdjackson Btw. Happy 2022 :)
I can rename isShutdown
to isConfigured
flag and ...
isConfigured
to true at the end of the initialize
method to ensure that it is true only when the initialization was successful.isConfigured
to false at the beginning of the shutdown
method.if (frameHandler == null)
by if (!isConfigured)
I took one more look at the code and I'm wondering if there is still a chance that frameHandler
is disposed just in the moment when sendCommand
is about to use it. What I mean is such a scenario:
sendCommand
and if (!isConfigured)
evaluates to false.shutdown
at the same time and sets isConfigured
to false (too late for thread 1). FrameHandler
is shut down and set to null before thread 1 is able to do its stuff.frameHandler
and runs into a NPE or another error.In such a case we would need to somehow synchronize frameHandler
. WDYT?
I can rename isShutdown to isConfigured flag and ...
This sounds fine to me.
In such a case we would need to somehow synchronize frameHandler. WDYT?
Yes, while this is a small opportunity, you are right that there exists a possible NPE here. Maybe we can synchronise on the isConfigured
flag (making it a Boolean
instead of boolean
) since framehandler
gets set to null
we can't use this. I don't think we need to synchonise all uses - a quick search of the code shows only a few instances of the frameHandler == null
check, and I think the only one that needs synchronising is the one you highlight.
So I'm happy with this, or something similar if you see a nicer alternative.
@cdjackson
I updated the PR as discussed. I would like to clarify one thing. In sendCommand
there is the statement lastSendCommand = System.currentTimeMillis();
. Since I added a isConfigured
check inside the thread, does it make sense to have the statement inside the thread as well?
Looking at this, I think the lastSendCommand = current time
statement should be inside the thread in any case. While in reality it probably doesn't make a lot of difference, if for some reason the network manager is sending down commands that don't get sent, it will currently hold off polling since lastSendCommand
gets set when sendCommand
is called, but then the command may not actually be sent to the NCP.
In the case of the change you've made, it probably matters even less since all it will do is hold off polling - even if we're not configured. But since we're not configured, polling doesn't matter...
Anyway, I think the answer is YES
- it would be better to move this statement down to where the command is really being sent - after all checks are done that may stop it being sent.
@TomTravaglino I see you pushed this change - do you consider this good to go now? If so I'll take a last look with a view to merging...
@cdjackson Sorry, I forgot to write a small comment. Yes, it's good to go.
Thanks @TomTravaglino
Resolves #1286
This PR introduces a flag in ZigBeeDongleTelegesis and ZigBeeDongleEzsp to check if the shutdown method was called. The flag is used in the sendCommand in order to avoid using a shutdown or terminated Executor.
@cdjackson Please take a look and share your opinion.