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: implementing retrying_publish wrapper for the relay client #124

Closed
wants to merge 1 commit into from

Conversation

geekbrother
Copy link
Contributor

@geekbrother geekbrother commented Oct 12, 2023

Description

This PR introduces retrying_publish function which wraps the current relay client .publish method and adds additional retrying and client reconnection logic to the current publishing mechanism:

  • Checking if there is an error when publishing a message,
  • Trying to reconnect with the 1-second retry interval in a loop,
  • After successfully reconnection trying to re-publish the message,
  • Give up when the publish errors after reconnecting.

The client.publish is replaced with the new function in websocket message handlers.

Resolves #74

How Has This Been Tested?

It should be tested by the current integration testing because the new function replaces the .publish which is indirectly used in the tests.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@geekbrother geekbrother self-assigned this Oct 12, 2023
@arein arein added the accepted label Oct 12, 2023
@geekbrother geekbrother force-pushed the max/fix/handle_disconnect_on_publish branch 2 times, most recently from 0d62c8e to d5a368a Compare October 12, 2023 12:47
@geekbrother geekbrother marked this pull request as ready for review October 12, 2023 12:52
@geekbrother
Copy link
Contributor Author

I don't like how connection is duplicated twice and this should be refactored according to DRY, but that can be a follow-up.
We already have a top-level reconnection logic, I'm sure it should be somewhere near it.

@geekbrother geekbrother force-pushed the max/fix/handle_disconnect_on_publish branch from d5a368a to 68e2c46 Compare October 12, 2023 13:38
@geekbrother geekbrother requested a review from Elyniss October 12, 2023 14:05
.await
{
warn!("Reconnecting after an error in publishing message: {}", e);
while let Err(e) = client
Copy link
Member

@chris13524 chris13524 Oct 12, 2023

Choose a reason for hiding this comment

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

Won't this conflict with the retry loop in websocket_service?

ttl: Duration,
prompt: bool,
) -> Result<()> {
if let Err(e) = client
Copy link
Member

@chris13524 chris13524 Oct 12, 2023

Choose a reason for hiding this comment

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

I think we need to get the type of error. Not all errors are disconnect related

@geekbrother geekbrother marked this pull request as draft October 13, 2023 11:35
@geekbrother
Copy link
Contributor Author

To handle properly with coming updates in #127 this implementation should be changed.
My thoughts about it and implementation should now look like below:

  1. wsclient: add is_connected: bool and connected_notifier: Arc<Notify> for the state of the connection and notifying.
  2. connect method spawning a detached (re)connection loop (in a Tokyo task) which changes the state of the connection when the connection is ready.
  3. Publish method abstraction:
    1. If is_connected==false wait for the connected_notifier ,
    2. Publish a message,
    3. Trigger the connect method if the publish throws an error and wait for the connected_notifier before the re-publishing attempt.

What do you think, @chris13524 ?

@geekbrother
Copy link
Contributor Author

Closing this PR in a favor of #128 128

@geekbrother geekbrother deleted the max/fix/handle_disconnect_on_publish branch November 8, 2023 11:04
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.

fix: handler functions do not anticipate websocket disconnection
3 participants