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

Shutdown and reclaim server-side IM resources on underlying session going away #18580

Open
mrjerryjohns opened this issue May 18, 2022 · 10 comments
Assignees
Labels

Comments

@mrjerryjohns
Copy link
Contributor

Problem

IM resources on the server today continue to exist and linger around even if their bound session has gone away.

Proposal

Register for the OnSessionReleased delegate in the IM engine perhaps, and iterate over all Read/Command/Write handlers and evict those that are bound to the matching session.

@yunhanw-google yunhanw-google self-assigned this May 24, 2022
@kghost
Copy link
Contributor

kghost commented May 27, 2022

Read/Command/Write commands are bound to ExchangeContext, there are no resource bound to OnSessionReleased, such that nothing should be handled inside OnSessionReleased.

The subscription has something related to session, which should be removed when OnSessionReleased is triggered. Synced with @yunhanw-google that even if the resource is not cleared in OnSessionReleased, the resources will be released later. On the client side, the subscription will be released when there is no report arrives for a while; On the server side, an error raised by ReadHandler::SendReportData will remove the subscription.

Handling OnSessionReleased in ReadHandler is better-to-have, to free resources as soon as possible.

@yunhanw-google
Copy link
Contributor

yunhanw-google commented May 27, 2022

per chat with @kghost , with his below change, when report messaged fails to send with crmp retires, session would not be released and would be kept alive. And Session would be released only when there is no enough resource in session pool. It seems this feature would not be that useful since session would be kept alive for long time.

#18107
#18878
@bzbarsky-apple @mrjerryjohns

@bzbarsky-apple
Copy link
Contributor

@mrjerryjohns When a session is actually released, we are going to close the relevant exchanges. So we don't need to listen for OnSessionReleased; we just need to listen for OnExchangeClosing, right?

@mrjerryjohns
Copy link
Contributor Author

So we don't need to listen for OnSessionReleased; we just need to listen for OnExchangeClosing, right?

For interactions that always have an open exchange, yes, this is true. For subscriptions specifically, this won't suffice.

@mrjerryjohns
Copy link
Contributor Author

Nonetheless, I think the utility of this is diminished since:

  1. Sessions won't really go away unless evicted to make way for a new one.
  2. If new sessions get created from the same peer, the subscription is likely to shift to the new one anyways.

Will see if this crops up as an issue during inter-op.

@yunhanw-google
Copy link
Contributor

@mrjerryjohns is this related to this ticket #21084?

@mrjerryjohns
Copy link
Contributor Author

No, that one is about deleting IM objects on fabric deletion. This is about deleting objects on session deletion..

@stale
Copy link

stale bot commented Feb 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Feb 12, 2023
@bzbarsky-apple
Copy link
Contributor

I think we need a clear idea of what's left to do here, which cases would be helped but this, etc, broken out by interaction type.

And perhaps separate issues by interaction type, since I expect the constraints and tradeoffs to be different...

@stale stale bot removed the stale Stale issue or PR label Feb 13, 2023
@stale
Copy link

stale bot commented Aug 13, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Aug 13, 2023
@bzbarsky-apple bzbarsky-apple removed the stale Stale issue or PR label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants