[regression] OH Core OAuth synchronization error

[andrewfg]

There is a synchronization issue in OH Core OAuth

#4686 (comment)

My initial suspicion was that the HTTP token refresh call was blocking and the HTTP client does not have a timeout on it.
However users have shown that the HTTP token refresh call does succeed, so the blocking is apparently within OH Core code.

So we need to fix both of these issues. I will open a PR soon.

[lsiepel]

With a 10 minute interval, the overhead of creating and closing a httpclient is little. And without looking at the code it would be strange that this may casue a lock.

The synchronized keywords to the two methods should not cause a lock from with Tado. As the thread gets a lock when calling the first method, it should not get a deadlock on the second as the thread already holds a lock to the object.

An explicit timeout should be added, but i don;t expect it to fix the issue.

Have not looked at the whole thread, need to read it all. There is another improvement that can be done: the thread name "OH-thingHandler-1" is not according to the convention. Not fixing the root cause, but can help troubleshooting. (same as the above timeout)

[andrewfg]

the thread name "OH-thingHandler-1" is not according to the convention

The thread name actually comes from elsewhere in OH Core related to -- well -- thing handlers (so if you want to fix that, it is another issue). Nevertheless its name was indeed a clue to the problem.

I believe the issue is because the binding thing handler calls getAccessTokenResponse(), which calls refreshToken() which via accessTokenRefreshListeners.forEach(l -> l.onAccessTokenResponse(accessTokenResponse)) calls the binding back, which causes an online status refresh, which causes another API call, which causes another request to getAccessTokenResponse() ad infinitum.

So I am working on a new PR openhab/openhab-addons#18481 where I will (among other things) break that loop from within the binding.

However I also made a PR #4699 towards OH core to add a timeout on the OAuth HTTP calls

[lsiepel]

Would it be possible to end the lock when the token is refreshed and call onAccessTokenResponse after the lock is released ?

1. the locking is the responsibility for the core, not the binding.
2. by making this the responsibility of the binding, a unwanted dependency is created
3. all future consumers of this core api would need to implement this fix

[andrewfg]

I don't think that is possible. (I already spent an hour or so playing around with it). I think it needs to put the call-backs on a separate thread, but the class doesn't have an executor service, so adding one would create extra overhead.

[andrewfg]

Solved via #4699