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

Add comments in SessionHandle explaining its intent #33271

Merged
merged 6 commits into from
May 7, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/transport/Session.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,48 @@ class Session;
* as pending removal but not actually removed yet. During this period, the handle is functional, but the underlying session
* won't be able to be grabbed by any SessionHolder. SessionHandle->IsActiveSession can be used to check if the session is
* active.
*
* A `SessionHandle` exists to guarantee that the object it points to will not be released while allowing passing
* the handle as a reference, to not incur extra reference-counted Retain/Release calls.
*
* Example code that breaks assumptions (hard to reason about/maintain):
*
* void Process(ReferenceCountedHandle<Session> &handle);
* class Foo {
* ReferenceCountedHandle<Session> mSession;
* void ResetSession() { mSession = createNewSession(); }
* void DoProcessing() {
* Process(mSession);
* }
*
* static Foo& GetInstance();
* };
*
* void Process(ReferenceCountedHandle<Session> &handle) {
* Foo::GetInstance()->ResetSession(); // this changes the passed in handle
* // trying to use "&handle" here may point to something else altogether.
* }
*
* Above would be fixed if we would pass in the handles by value, however that adds extra code
* to call Retain/Release every time. We could also by design say that passed in references will
* not change, however historically the codebase is complex enough that this could not be ensured.
*
* The end result is the existence of SessionHandle which is NOT allowed to be held and it serves
* as a marker of "Retain has been called and stays valid". The code above becomes:
*
* void Process(SessionHandle &handle);
*
* ....
* void Foo::DoProcessing() {
* SessionHandle handle(mSession); // retains the session and mSession can be independently changed
* Process(&handle); // reference is now safe to use.
* }
*
* To meet the requirements of "you should not store this", the Handle has additional restrictions
* preventing modification (no assignment or copy constructor) and allows only move.
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
* NOTE: `move` should likely also not be allowed, however we need to have the ability to
* return such objects from method calls, so it is currently allowed.
*
*/
class SessionHandle
{
Expand Down
Loading