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

Forever reconnect to other clients upon failure #909

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

fredo
Copy link
Contributor

@fredo fredo commented Jan 26, 2021

This PR introduces the ClientManager which controls the status of matrix clients to their respective home server. Each home server has a connect_client_worker which will trigger the creation of a new client if the connection to the homeserver fails.

A reconnect is described by creating a new client object, logging into the server, joining broadcast rooms and starting the greenlet listen_forever.

@auto-assign auto-assign bot requested a review from ulope January 26, 2021 18:21
@fredo fredo force-pushed the presence-by-server branch from 5bf3029 to d295088 Compare January 26, 2021 20:46
@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #909 (3dd8984) into master (b5d0f81) will decrease coverage by 0.11%.
The diff coverage is 75.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #909      +/-   ##
==========================================
- Coverage   90.38%   90.27%   -0.12%     
==========================================
  Files          41       41              
  Lines        3057     3104      +47     
  Branches      389      388       -1     
==========================================
+ Hits         2763     2802      +39     
- Misses        209      214       +5     
- Partials       85       88       +3     
Impacted Files Coverage Δ
src/raiden_libs/matrix.py 68.85% <73.45%> (+1.85%) ⬆️
src/raiden_libs/utils.py 93.87% <86.66%> (+2.57%) ⬆️
src/pathfinding_service/service.py 91.96% <100.00%> (ø)
src/request_collector/server.py 85.33% <100.00%> (ø)
src/raiden_libs/blockchain.py 85.71% <0.00%> (-0.90%) ⬇️
src/pathfinding_service/api.py 95.68% <0.00%> (+0.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7af66a...3dd8984. Read the comment docs.

Copy link
Contributor

@ulope ulope left a comment

Choose a reason for hiding this comment

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

LGTM

One question below.


except RuntimeError as ex:
if available_servers is None:
raise ex
Copy link
Contributor

Choose a reason for hiding this comment

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

To re-raise just raise is enough and will preserve the original traceback.

Comment on lines +390 to +399
while not self.stop_event.is_set():
stopped_client = self.server_url_to_other_clients.pop(server_url, None)
if stopped_client is not None:
self.user_manager.remove_client(stopped_client)
try:
client = self._start_client(server_url)
assert client.sync_worker is not None
client.sync_worker.get()
except (TransportError, ConnectionError):
log.debug("Could not connect to server", server_url=server_url)
Copy link
Contributor

@ulope ulope Feb 16, 2021

Choose a reason for hiding this comment

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

Hm unless I'm missing something this looks to me like it will re-connect to the server in each loop without checking if the connection is actually dead?
Ok I did miss the client.sync_worker.get().

Also this probably needs a lock so the state of the server_url_to_other_clients dict is consistent.

fix test

fix circular dependency

test for client connect forever

fix lint

only fetch server list if it is chainID supported by RSB

raise
@fredo fredo force-pushed the presence-by-server branch from 14a9d31 to 438516e Compare February 16, 2021 16:46
fix client manager test
@fredo fredo merged commit e2ebac0 into raiden-network:master Feb 16, 2021
@fredo fredo mentioned this pull request Feb 16, 2021
@fredo fredo deleted the presence-by-server branch February 16, 2021 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Try to reconnect to other client if failed
2 participants