Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix websocket automatic reconnection #812

Merged
merged 2 commits into from Mar 23, 2024
Merged

Conversation

smcmnz
Copy link
Contributor

@smcmnz smcmnz commented Jan 27, 2024

Changes the reconnection approach to use the reconnect function of the websocket-client library. The current approach throws an error but does not reconnect.

After reconnection a small delay was needed before the session capabilities are registered otherwise the client will not appear as user controllable.

Resolves #797

@@ -65,4 +65,4 @@ dependencies:
- addon: 'script.module.addon.signals'
version: '0.0.5+matrix.1'
- addon: 'script.module.websocket'
version: '0.58.0+matrix.1'
version: '1.6.4'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you verified that this version of the websocket library actually exists in the Kodi repositories for the supported versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this version is available for Matrix, Nexus and Omega

@@ -62,23 +61,18 @@ def run(self):

while not self.stop:

time.sleep(self.retry_count * 5)
self.wsc.run_forever(ping_interval=10)
self.wsc.run_forever(ping_interval=10, reconnect=10)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the main functional change in this PR is the addition of the reconnect parameter here.

Could you elaborate on the reasoning behind removing the back-off delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reconnect param makes the reconnect logic handled by the websocket library. I added logging to the backoff delay and found it wasn't getting called anymore so I removed it for clarity.

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 21.52%. Comparing base (e478e4b) to head (e234fb4).
Report is 9 commits behind head on master.

❗ Current head e234fb4 differs from pull request most recent head 3d24ca8. Consider uploading reports for the commit 3d24ca8 to get more accurate results

Files Patch % Lines
jellyfin_kodi/jellyfin/ws_client.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #812      +/-   ##
==========================================
+ Coverage   21.50%   21.52%   +0.02%     
==========================================
  Files          63       63              
  Lines        8546     8537       -9     
  Branches     1572     1571       -1     
==========================================
  Hits         1838     1838              
+ Misses       6684     6675       -9     
  Partials       24       24              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glogiotatidis
Copy link

Is there something we can do to push this fix fwd?

Copy link

sonarcloud bot commented Feb 29, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@IosifZ
Copy link

IosifZ commented Mar 22, 2024

Just discovered Jellyfin (moving away from Plex) and this little issue is driving me crazy :)
Any timeline on when this issue will be solved?

Copy link
Member

@mcarlton00 mcarlton00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick test it works after restarting my server. The old method did too, but this picked it up faster

@mcarlton00 mcarlton00 merged commit b880f47 into jellyfin:master Mar 23, 2024
9 checks passed
@smcmnz smcmnz deleted the ws-reconnect branch March 24, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Websocket does not reconnect after Jellyfin server restart
5 participants