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

Allow clients to cancel a remote call and for the signal to properly propagate through the handler #77

Open
aryanjassal opened this issue Dec 16, 2024 · 1 comment
Assignees
Labels
development Standard development

Comments

@aryanjassal
Copy link
Member

aryanjassal commented Dec 16, 2024

Specification

The cancel parameter exposed in the handler actually cancels the underlying transport stream. This is needed so the middleware can cancel the stream without needing access to the ends of the stream. However, this is not strictly necessary for handlers, and the higher-level handlers should not have access to the underlying webstream implementation. Maybe ClientHandler and DuplexHandler should still provide a cancel to the handler, which would abort the incoming stream from the client instead of closing the underlying webstream.

The return value for the client is all a Promise. This means that the client can't really cancel the stream by doing pC.cancel(). Even generating and passing in a new context will not work, as the contexts for client call and the server handler are isolated.

Actually, PromiseCancellable implements exactly this, but it hasn't been properly incorporated into the project, as only RPCServer.ts uses it, and it is not listed as an explicit dependency of the project. So, it needs to be listed as a dependency and be used in RPCClient.ts in the return types of a caller.

To properly implement cancellation, the client can cancel the function call by using f.cancel() and optionally provide a reason. The reason can be an error, in which case the error will get converted into a code which corresponds uniquely to that error. Then, that code can be sent over the transport layer to the server, which can then know which error was thrown based on the code. If the code has not been recognised, then a generic error can be thrown.

To build this new error codes, we can separate the error codes to transport errors and application errors. Transport errors happen when the transport layer itself encounters an error. Application errors should happen on the application layer itself. Strings can't be serialised as the error codes are 32-bit integers.

If this cancellation has been done from the client, the RPC should update the handler context and send it an abort signal. While the underlying stream would have been closed upon abortion, the handler would still be able to keep running to perform cleanup.

The behaviour of cancellation wasn't clear in the code, and it relied on either tests and existing knowledge of the code base. This is not good. The code should have documentation and it should also be mentioned in the README.

Other discussions

Another idea was explored to handle cancellation, where the context passed to the client caller should be passed through to the server itself, too. However, this solution had some issues, like timeout is modified depending on the server and client timeout values. Moreover, the contexts won't actually be connected on the client and handler, so an update message or an abort message needs to be sent to synchronise the contexts. However, unary handlers are designed to take in only a single message and not multiple messages, so this would result in a massive code overhaul and other issues, so it doesn't make much sense to implement this solution.

Additional context

Tasks

  1. Add async-cancellable to package.json
  2. Properly integrate PromiseCancellable in both RPCServer and RPCClient
  3. Remove cancel parameter from the high-level handler and the caller, keeping it for duplex and client streams if needed
  4. Allow cancelling a function call from the client easily
  5. Improve documentation for cancellation in both the code and the README
  6. Update the tests to explicitly test cancellation 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

1 participant