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

Add call retry option to retry calls which fail due to connection error #7

Closed
mikuso opened this issue Apr 28, 2022 · 3 comments
Closed
Labels
enhancement New feature or request

Comments

@mikuso
Copy link
Owner

mikuso commented Apr 28, 2022

It would be good to have an option to opaquely retry calls which fail due to connection-related errors.

Perhaps a call option {maxRetries: 3} and/or an option in the RPC client/server constructors to set a default.

By default, maxRetries should probably be 0 since there's a chance that retries could have undesirable effects if the call has been handled.

@mikuso mikuso added the enhancement New feature or request label Apr 28, 2022
@mikuso
Copy link
Owner Author

mikuso commented Apr 28, 2022

If call is retried and still fails, what error should be returned?

The first error? last error? or an AggregateError?

@mikuso
Copy link
Owner Author

mikuso commented Apr 28, 2022

As per #8, the retried calls should reuse the same message ID as the initial call.

Reasons for this:
* If the callee enforces the spec requirement that message IDs should not be reused, it reduces the chance of causing unintended side-effects on retry.
* If the callee supports idempotency, it can identify the repeated call from the message ID.

Scratch that. Re-using the same message ID makes no sense after losing a connection, as the repetition of a message ID has no significance when sent over a new websocket connection.

Further to that, re-using the same message ID within a single session is a violation of the spec and should trigger a "AttemptedReplayAttacks" security event if it should occur.

The only time it would be appropriate to retry sending a message is if the websocket connection closes after sending the message, but before receiving a response. But even then, there's no good reason to want to re-use a message ID.

@mikuso
Copy link
Owner Author

mikuso commented Jan 25, 2023

After lengthy consideration, I think this feature may cause more harm than good, so I'm going to retire this idea for now.

  • There are only limited circumstances where retrying a call would make sense, and wouldn't really be feasible to implement for server-initiated calls.
  • The term "retry" could be confusing, as we're really only intending to retry calls which fail due to connection issues, and not to retry calls which fail due to any other numerous possible reasons, such as incomplete implementations, timeouts, etc...
  • If we can't be confident whether the initial call succeeded or failed, it shouldn't be for this library to decide how to handle this edge case.
  • Some server implementations won't allow calls to be made upon initial connection until a boot notification has been accepted. This means the retried call could lead to further errors and confusion when debugging.
  • Since idempotency is not supported by the spec, I don't think there is need to implement this in the library.

@mikuso mikuso closed this as completed Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant