MatrixAI / Polykey

Polykey Core Library
https://polykey.com
GNU General Public License v3.0
31 stars 4 forks source link

Review and refactor RPC handlers to handle cancellation properly #832

Open tegefaulkes opened 1 month ago

tegefaulkes commented 1 month ago

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.
linear[bot] commented 1 month ago

ENG-432 Review and refactor RPC handlers to handle cancellation properly

tegefaulkes commented 2 weeks ago

@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.

aryanjassal commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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