Bugfix/sonos oauth reconnect #2345
Conversation
|
Channel deleted. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 0095654 |
NoahCornell
force-pushed
the
bugfix/sonos-oauth-reconnect
branch
from
b688780 to
42d8093
Compare
NoahCornell
requested review from
cjswedes,
nickolas-deboom,
tpmanley and
varzac
| local result, err = security.get_sonos_oauth() | ||
| if math.abs(expiration - now) < PREEMPTIVE_OAUTH_REFRESH_PERIOD_S then | ||
| -- Passing in true here forces refresh of the token. | ||
| local result, err = security.get_sonos_oauth(true) |
There was a problem hiding this comment.
Can you quantify what the API pressure was before when compared with what it is now? We probably should get someone who wrote those apis to signoff on this. IIRC there was discussion of when force_refresh was appropriate to use, and I worry that this may be too aggressive and cause undue stress on our cloud and the sonos auth service.
There was a problem hiding this comment.
Talking with Ingvar it sounds like there are some reasons why we don't want to use force refresh here so I'll remove that commit because the other commit is still valid and a bigger issue. I'll open up a new PR to deal with the token expiration issue and missing capability commands.
There is a race condition when a sonos connection calls close on itself. It immediately calls on_close which will spawn a task to reconnect. The reconnect task checks if the connection is running which depends on the socket being closed and cleaned up. Instead, call on_close after the socket has been closed and cleaned up.
NoahCornell
force-pushed
the
bugfix/sonos-oauth-reconnect
branch
from
42d8093 to
0095654
Compare
Check all that apply
Type of Change
Checklist
Description of Change
This fixes two issues regarding sonos oauth expiration.
**Better Preemptive Token Refreshing:**~~This is ensuring that we correctly preemptively refresh the oauth token. We were not setting
refreshtotruewhen calling security.get_sonos_oauth. Setting this totrueends up setting a query parameter to force refresh the token when making the API calls to get an ISA token. ~~This change also increases the period where we will attempt to refresh the token to 2 hours. This is done to match up with the SSDP event handler which checks auth hourly so we should never end up in a situation where the token expires. If the token does expire, then our next call on the websocket will fail and we will not handle the next capability command.
Removed due to potential issues with force refresh. Will open a new PR to handle.
Reconnect Task Race Condition:
If we don't have the proper auth to make websocket requests, then we will receive a
ERROR_NOT_AUTHORIZEDerror. The handling for this error calls Router.close_socket_for_player.Router.close_socket_for_playerwill immediately call theon_closecallbacks for any listeners. For the sonos_connection, this calls _spawn_reconnect_task.Both the legacy and oauth flavors of the reconnect task check if the connection is running before attempting to reconnect. This ends up checking in the websocket router if we are connected.
Since there are several tasks at play here, we might do the check if we are connected before this runs. The reconnect task will then exit early since it appears as we are already connected. The fix is to call the
on_closewhen the socket is actually closed, rather than immediately, so that the reconnect task doesn't exit early.Summary of Completed Tests
I have been testing by using the hedge endpoints to manually set the token and it's expiration. I can let it sit and let the expiration naturally hit as well to make sure things are working.
on_closestill only called a single time. Reconnects successfully when plugged back in.