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

Review and refactor RPC handlers to handle cancellation properly #832

Open
tegefaulkes opened this issue Oct 18, 2024 · 6 comments · May be fixed by #846
Open

Review and refactor RPC handlers to handle cancellation properly #832

tegefaulkes opened this issue Oct 18, 2024 · 6 comments · May be fixed by #846
Assignees
Labels
development Standard development

Comments

@tegefaulkes
Copy link
Contributor

Specification

Some RPC handlers are preforming long running async tasks. In these cases they need to make use of the provided ctx and handle cancellation gracefully to prevent any resource leaks. A clear example of this is the vaults pull and clone handlers.

So we need to review handlers and update the ones that need CTX handling applied.

Additional context

Tasks

  1. Review all RPC handlers and identify ones that preform complex async or long running tasks.
  2. Refactor theses handlers to handle cancellation in a responsive and graceful manor.
  3. Apply testing to see if timing out during these tasks causes any problems. Possibly apply regression testing for this.
@tegefaulkes tegefaulkes added the development Standard development label Oct 18, 2024
@tegefaulkes tegefaulkes self-assigned this Oct 18, 2024
Copy link

linear bot commented Oct 18, 2024

@tegefaulkes
Copy link
Contributor Author

@aryanjassal You'll need to take this over. There is existing work done in branch feature-eng-432-review-and-refactor-rpc-handlers-to-handle-cancellation relating to this.

Copy link
Member

aryanjassal commented Nov 7, 2024

For handlers like VaultsSecretsGet (as of Polykey#838), it is a ServerHandler, but it streams back the file contents. As such, there is no loop anywhere. For cases like that, where should the context handling be put?

Similarly, I don't think we can actually handle cancellation for ClientHandlers or UnaryHandlers as they don't send back streamed data, so cancellaton doesn't make much sense.

@tegefaulkes
Copy link
Contributor Author

Cancellation applies to async operations and not just streamed data. For the most part the streaming handlers are the biggest targets for handling cancellation. But anything that waits for locks or takes a little while to do a thing can be cancelled.

For example, the fetch() call is considered a unary function, but it has cancellation via an AbortSignal because a fetch operation could take a long time.

@CMCDragonkai
Copy link
Member

For handlers like VaultsSecretsGet (as of Polykey#838), it is a ServerHandler, but it streams back the file contents. As such, there is no loop anywhere. For cases like that, where should the context handling be put?

Similarly, I don't think we can actually handle cancellation for ClientHandlers or UnaryHandlers as they don't send back streamed data, so cancellaton doesn't make much sense.

Anything asynchronous can be cancelled. Streaming is orthogonal.

@CMCDragonkai
Copy link
Member

I think the docs is lacking info about the key concepts in RPC.

@aryanjassal aryanjassal linked a pull request Nov 19, 2024 that will close this issue
97 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
Development

Successfully merging a pull request may close this issue.

3 participants