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

Server stream progress updates for RPC calls that have long running async operations #264

Open
tegefaulkes opened this issue Aug 23, 2024 · 5 comments
Labels
development Standard development

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Aug 23, 2024

Specification

In the case of vaults clone we have a unary call. The call will be active for the duration of the cloning process which in the background is a compound operation of complex streams. For a very large vault we run into a problem with the client level unary call timing out before we can complete the call it is timing out.

To fix this we need to change the unary call to a server streaming call. We then need to stream over progress updates to reset the timeout timer to prevent the time out. In the case of vaults clone this means sending over the cloning process periodically. Either every 5% or every few seconds or so. Then when the clone is complete we send over details such as the vault name and the new vault ID for that cloned vault. For most cases we can't really know what the absolute progress is. But we can output the amount of arbitrary progress made. This progress must be output on stderr like any other feedback output.

This change to streaming a progress updates need to apply to and client RPC call that waits for a complex or long running task. As far as I can tell this just applies to the vaults cloning, pulling and cross signing claims. But we'll need to investigate this deeper.

Additional context

Tasks

  1. Identify all client RPC calls that wrap complex long running operations
  2. Refactor these RPC calls to be a server stream call that sends over progress updates. The details of the progress isn't actually important so much that they reset the timeout for the call.
@tegefaulkes tegefaulkes added the development Standard development label Aug 23, 2024
@CMCDragonkai
Copy link
Member

image

Basically we need to stream progress to STDERR to avoid a ErrorRPCTimeout, but those bugs #185 and #198 need to be fixed too.

@CMCDragonkai
Copy link
Member

@tegefaulkes mentioned that the git cloning process doesn't have an abort system. So if we timeout, it's likely the cloning continues, and the connection is leaked.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Aug 24, 2024

This problem was actually discussed earlier here #74 and also connected to MatrixAI/js-rpc#52 and MatrixAI/js-rpc#57.

@tegefaulkes you even said:

Any commands that attempt a connection such as nodes add or vaults clone could take a long time to complete, especially if we fail to connect. The RPC timeout is 15 seconds with the connection timeout being the same 15 seconds but could take multiple 15 second attempts before giving up. Worst case is about 20/3 attempts to fail a nodesFind. This will cause a RPC timeout before we can respond with the actual error.

However using the grace timer solution is not sufficient. As in MatrixAI/js-rpc#59.

That assumed that the agent side would be able to tell the client side a richer error if the client side timed out.

However we understand that some operations like vaults clone might just take a long time. In which case, timeouts like the regular 15 seconds shouldn't apply like this. Grace timer can still work in the case of true unary operations. But vaults cloning and pulling are not. They are like download operations that may take arbitrary time. So that's why we can identify these operations as something requiring a streaming call instead.

Copy link
Contributor Author

I also need to note that the RPC handlers need to properly handle CTX and abortion properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
Development

No branches or pull requests

2 participants