[lgwebos] Avoid thing updates when the thing handler is already disposed

[lolodomo]

Signed-off-by: Laurent Garnier lg.hc@free.fr

[TravisBuddy]

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[cpmeister]

LGTM

[lolodomo]

Ok, I just updated the PR, the problem is now handled by the thing handler.

[TravisBuddy]

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[sprehn]

still trying to understand the issue.

OH initializes the handler
the reconnect job starts
once the socket connection can be established binding sends "hello" message to TV
tv responds with hello response
binding calls config.storeProperties(map);
binding sends "register" message to TV
tv responds with register response
binding calls config.storeKey(key);

I agree that we could optimize both storeProperties and storeKey to only actually update if values changed. It may already solve the problem, as these values are only set once at the beginning.

storeKey today is already designed not to go through re-initialization. Is storeProperties the problem?

Or is the problem, that the the connection sequence continues running, while the binding disposes, calls disconnect and LGWebOSTVSocket is in state DISCONNECTED. If so, what we could do is to stop the sequence by putting entry conditions into the connection sequence of LGWebOSTVSocket::onMessage

// add additional state CONNECTING

switch (response.getType()) {
....
            case "hello":
               if (state != CONNECTING) {
                    logger.debug("Skipping resonse {}, not in CONNECTING state, state was {}", message, state);
                    break;
                }
....
                break;
            case "registered":
               if (state != REGISTERING) {
                    logger.debug("Skipping resonse {}, not in REGISTERING state, state was {}", message, state);
                    break;
                }
  ....
              break;
        }

[lolodomo]

Or is the problem, that the the connection sequence continues running, while the binding disposes

Yes, you got it.

Your option looks good to me, it is more or less the same as my previous test "listener == null" but with a really cleaner solution.
If a new state CONNECTING is introducing, we just have to take care of it in the thing handler.
I will change the PR to use this way to implement the fix.

[lolodomo]

@sprehn : please review this new version.

[sprehn]

LGTM

[lolodomo]

@sprehn : just one question/detail: we stop reconnection job and start keep alive job when the state is changed to REGISTERING. Should we rather do it now when the state is changed to CONNECTING ?

[lolodomo]

@cpmeister : can you start a new review please ? The PR was largely changed since your first approval.

[sprehn]

@sprehn : just one question/detail: we stop reconnection job and start keep alive job when the state is changed to REGISTERING. Should we rather do it now when the state is changed to CONNECTING ?

Good point.

It is better to stopReconnectJob() in CONNECTING.
but startKeepAliveJob should actually move to REGISTERED.

[lolodomo]

Change with jobs done

[TravisBuddy]

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

[cpmeister]

LGTM

[cpmeister]

Manually approved sign-off