Cancel subscribeToLogs and messageStream RPCs when the Session closes #5433
Conversation
This PR fixes subscribeToLogs and messageStream RPCs; previously, they were staying open when a Session closed. Session closing behavior for a number of RPCs was specifically added as a tests here. I'll note that we are inconsistent in how we handle this case - some RPCs will complete successfully, others will pass along UNAUTHENTICATED or CANCELLED. We should aim to clean this up, but I'm proposing we do that as a follow-up PR to avoid PR bloat. The fix involves hooking into SessionServiceCallListener, which is a centralized place where we maintain Session state information. We should strongly consider migrating other RPC Session close behavior to here to simplify the implementations and maintain consistency. Again, I'd suggest doing that as a separate PR. Fixes deephaven#5415
devinrsmith
added
NoDocumentationNeeded
NoReleaseNotesNeeded
|
I'll make the argument that completing an RPC when a Session closes is the wrong behavior; the server completing an RPC only tells the client "the server is done sending messages" (half-close) - in the case of a bidir or client streaming RPC, the client could still keep the RPC up as long as they wanted. |
| context, | ||
| session, | ||
| errorTransformer, | ||
| CANCEL_RPC_ON_SESSION_CLOSE.contains(serverCall.getMethodDescriptor().getFullMethodName())); |
There was a problem hiding this comment.
What if we did !serverCall.getMethodDescriptor().getType().serverSendsOneMessage()?
There was a problem hiding this comment.
I'm definitely in favor of adding more RPCs into auto cancel; that said, I'm trying to be cautious about changing existing behavior without further testing and validation.
I think we actually want this to be try for all RPCs, even ones where the server sends one message. As a real example,
/*
* Receive a best-effort message on-exit indicating why this server is exiting. Reception of this message cannot be
* guaranteed.
*/
rpc TerminationNotification(TerminationNotificationRequest) returns (TerminationNotificationResponse) {}is a long-living RPC where the server only sends one message; I would want this to be auto cancelled as well. (Right now, it does have its own cancellation logic, but I think it could be automated here as well.)
There was a problem hiding this comment.
Hmm that makes sense. That's kind of annoying; maintaining a white list is fragile, maintaining a black list would be annoying for "power users" who inject additional gRPC services.
There was a problem hiding this comment.
I actually think we could safely move to auto cancel for every RPC attached to a Session (with a bit of due diligence to consider downstream effects). I don't think it's an unreasonable position for additional gRPC services that are implemented and are bound to a Session to inherit this behavior.
There was a problem hiding this comment.
#5534 created for follow-up to this point.
| super(delegate); | ||
| this.call = call; | ||
| this.context = context; | ||
| this.session = session; | ||
| this.errorTransformer = errorTransformer; | ||
| this.autoCancelOnSessionClose = autoCancelOnSessionClose; | ||
| if (autoCancelOnSessionClose && session != null) { |
There was a problem hiding this comment.
I think we might actually want to error if no session exists and only allow server-streaming rpc's if there is a session to attach to them.
There was a problem hiding this comment.
That's a good idea, to add a safety check in the code path to here - I expect that assumption is already met today by our impls, but I can't guarantee it. I don't think that immediately absolves our server impls from still being a bit explicit (impls still need to be explicit and call io.deephaven.server.session.SessionService#getCurrentSession to verify). It might be worthwhile to capture these expectations as annotations at the RPCs layer to make our server impls simpler. (I would expect that an RPC would need an annotation to opt-out; assume Session is necessary by default.)
There was a problem hiding this comment.
I have a commit that accomplishes this extra safety check, but I think it would be best as an immediate follow-up PR.
There was a problem hiding this comment.
Follow-up issue, #5535
This PR fixes subscribeToLogs and messageStream RPCs; previously, they were staying open when a Session closed. Session closing behavior for a number of RPCs was specifically added as a tests here. I'll note that we are inconsistent in how we handle this case - some RPCs will complete successfully, others will pass along UNAUTHENTICATED or CANCELLED. We should aim to clean this up, but I'm proposing we do that as a follow-up PR to avoid PR bloat.
The fix involves hooking into SessionServiceCallListener, which is a centralized place where we maintain Session state information. We should strongly consider migrating other RPC Session close behavior to here to simplify the implementations and maintain consistency. Again, I'd suggest doing that as a separate PR.
Fixes #5415