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

V2: Ensure that a signal exists for a completed RPC (regardless of error or success) #1253

Closed
davidfiala opened this issue Sep 30, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@davidfiala
Copy link

In the changelog of v2.0.0-beta.1 I saw:

Don't trigger handler signal on success by @srikrsna-buf in #1234

AFAICT, this implies that at present the server has no universal built-in way to detect when an RPC completes, regardless of reason. Thus resource cleanup cannot occur.

Per discussion in #1117 (comment) that there was a proposal to add a done signal and a settled signal. One I believe would always fire, and the other only on success. (As an aside, I'll share that the naming of those signals is not intuitive. Maybe settled and success would have been better?)

My concern is that without a mechanism for detecting doneness in all cases, resource cleanup will be very hard for developers, and they'd have to make their own try/catch wrappers... and that still might pose challenges if an interceptor crashes out of their control.

For reference, gRPC has discussed this in the past, and they made the overall decision that there's only one signal, which fires regardless of how the RPC ends: grpc/grpc-node#2771

I like the gRPC approach because it's simple and catches the most important usecase (cleanup).

I'd propose that before v2 leaves beta we either:

  1. ensure that we've added both signals to all contextes -- which will cost more overhead -- , or
  2. mirror gRPC behavior (aka, your v1 behavior) and only provide one signal that always fires. if people want special behavior (success-only signals), then they can write an interceptor themselves?

Thanks!

@davidfiala davidfiala added the enhancement New feature or request label Sep 30, 2024
@timostamm
Copy link
Member

Hey David, thanks for the issue. We'll look into this before we cut a stable version.

@timostamm
Copy link
Member

Fixed by #1282

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

2 participants