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

Need clearer smart-pointer like constructs for session management #18399

Closed
mrjerryjohns opened this issue May 12, 2022 · 3 comments
Closed

Need clearer smart-pointer like constructs for session management #18399

mrjerryjohns opened this issue May 12, 2022 · 3 comments
Assignees
Labels
stale Stale issue or PR v1.1

Comments

@mrjerryjohns
Copy link
Contributor

Problem

Our session management logic is quite complicated and difficult to understand on first read, especially when figuring out how session ownership/lifetime is managed.

A big part of the reason is that SessionHandle and SessionHolder don't quite behave like the smart pointer equivalents that other people are used to in STL.

A SessionHandle is clearly a strong-ref pointer (like a shared_ptr). Having a SessionHandle guarantees that the underlying object will not be reclaimed till the handle itself is. It is a bit different from a shared_ptr in that the object in the handle cannot be released ever (unlike shared_ptr::reset()).

A SessionHolder on the other hand, behaves like a weak_ref - just merely holding onto one doesn't mean the underlying object won't be reclaimed from under-neath whomever owns that holder. In fact, it can (if OnSessionReleased is called on the session). You're required to check if it has a valid reference before attempting to convert it to a strong ref, i.e get back a session handle. This is very similar to how std::weak_ref behaves too.

However, there are a couple of oddities that make this murky:

  • SessionHolder itself is also participating in the ref-counting of the underlying object. This suddenly makes it also look like a strong_ref...
  • When clients create unauthenticated sessions, they rely on the ref-counting behavior in SessionHolder to 'keep the object alive'.
  • When clients create un-authenticated sessions, they actually rely on the resultant ExchangeContext's SessionHolder to ref-count the session so that if anything wrong happens on the exchange, it will tear down the sesssion.

Proposal

  • Create a new SessionSharedPtr that is essentially a SessionHandle, but permits the underlying object to be released. This is exactly like how shared_ptr behaves.
  • Have SessionHandle be derived from SessionSharedPtr as a protected inheritance.
  • Rename SessionHolder to SessionWeakRef.
  • Make SessionWeakRef not actually do ref-counting, and instead, just have a raw vanilla pointer to Session.
  • Permit SessionSharedPtr to be held long-term (and not just a temporary on stack), so that entities doing the initial setup of a session can use that to automatically manage clean-up if something fails.
  • Once a session has been established, everyone (other than SessionMgr) should be holding SessionWeakRefs to that session. This is because they no longer own the lifetime of that session anymore, and it is up to the SecureSessionMgr to manage evictions of that session as needed.
  • Fix the unauthenticated session establishment logic for PASE to have the protocol/device controller layer own the session (by having a SessionSharedPtr reference to the session) and not rely on the ExchangeContext to do that.
@mrjerryjohns
Copy link
Contributor Author

Likely too risky to do for 1.0 at this stage in the game.

@stale
Copy link

stale bot commented Dec 20, 2022

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 Dec 20, 2022
@stale
Copy link

stale bot commented Dec 30, 2022

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issue or PR v1.1
Projects
None yet
Development

No branches or pull requests

3 participants