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

SERVER: subscribeToLogs RPC does not respond to session cancellation #5415

Closed
devinrsmith opened this issue Apr 25, 2024 · 2 comments · Fixed by #5433
Closed

SERVER: subscribeToLogs RPC does not respond to session cancellation #5415

devinrsmith opened this issue Apr 25, 2024 · 2 comments · Fixed by #5433
Assignees
Labels
bug Something isn't working grpc

Comments

@devinrsmith
Copy link
Member

During an audit of client / server behavior wrt Session#close, ManagedChannel#shutdown/Now, it was observed that the server implementaion of subscribeToLogs allows the RPCs for closed sessions to remain indefinitely. This is in contrast to a similar listFields RPC with does register itself with SessionState#addOnCloseCallback.

@devinrsmith
Copy link
Member Author

Likely introduced in #3379 due to the move away from io.deephaven.server.session.SessionCloseableObserver.

devinrsmith added a commit to devinrsmith/deephaven-core that referenced this issue Apr 26, 2024
@rcaudy rcaudy modified the milestones: 3. Triage, 4. Unscheduled Apr 26, 2024
@rcaudy rcaudy added grpc and removed triage labels Apr 26, 2024
@devinrsmith devinrsmith self-assigned this Apr 26, 2024
devinrsmith added a commit to devinrsmith/deephaven-core that referenced this issue Apr 26, 2024
@devinrsmith
Copy link
Member Author

ObjectService.messageStream has a similar issue

devinrsmith added a commit to devinrsmith/deephaven-core that referenced this issue Apr 30, 2024
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 a commit that referenced this issue May 15, 2024
…#5433)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working grpc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants