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 rmb calls timeout #202

Merged
merged 3 commits into from
Dec 11, 2024
Merged

fix rmb calls timeout #202

merged 3 commits into from
Dec 11, 2024

Conversation

Nabil-Salah
Copy link
Contributor

@Nabil-Salah Nabil-Salah commented Nov 28, 2024

Description

added exponential backoff and app exit when the listener thread is down

Changes

src/bins/rmb-relay.rs file

Related Issues

issue#200

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring

@Nabil-Salah
Copy link
Contributor Author

How we tested our approch:

first we ran an rmb-relay locally on my device on a successful runs connecting to the chain

Screenshot from 2024-11-28 14-52-45

we started another rmb-peer connected to the local relay

Screenshot from 2024-11-28 14-54-25

the redis database shows a successful change

Screenshot from 2024-11-28 14-53-56

after that I managed to cut the relay connections to the chain

Screenshot from 2024-11-28 14-53-21

in another remote device I ran rmb-peer with different twin id and changed the relay to a local relay

image

then I released block on the connection in the previous step

Screenshot from 2024-11-28 14-53-04

and it catches the peer change in the relay

Screenshot from 2024-11-28 14-55-00

sameh-farouk
sameh-farouk previously approved these changes Dec 4, 2024
Copy link
Member

@sameh-farouk sameh-farouk left a comment

Choose a reason for hiding this comment

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

The changes introduce valuable improvements overall. However, the loop remains infinite without external exit control or mechanisms such as a termination or shutdown signal. Upon reviewing the code base, it appears that the RMB relay lacks a fully implemented graceful shutdown mechanism:

  1. The main relay server loop in src/relay/mod.rs runs in an infinite loop without handling shutdown.
  2. The federation worker also operates in an infinite loop without shutdown handling.
  3. The event listener in this PR similarly lacks shutdown handling.

I recommend implementing proper graceful shutdown handling to ensure reliable operation. However, it would be more reasonable to address this in a separate PR.

Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Signed-off-by: nabil salah <nabil.salah203@gmail.com>
Copy link

@Omarabdul3ziz Omarabdul3ziz left a comment

Choose a reason for hiding this comment

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

good job ya nabil, i have small comments

src/bins/rmb-relay.rs Outdated Show resolved Hide resolved
src/bins/rmb-relay.rs Outdated Show resolved Hide resolved
src/bins/rmb-relay.rs Outdated Show resolved Hide resolved
@Omarabdul3ziz
Copy link

i tired and it do retry with backoff, but can we extend the timeout? maybe 5 min

Signed-off-by: nabil salah <nabil.salah203@gmail.com>
@ashraffouda ashraffouda merged commit 0ef17e7 into main Dec 11, 2024
1 check passed
@ashraffouda ashraffouda deleted the fix_rmb_calls_timeout branch December 11, 2024 14:12
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.

4 participants