Skip to content

Commit 242accd

Browse files
andy31415andreilitvinbzbarsky-apple
authored
Add comments in SessionHandle explaining its intent (#33271)
* Explain in comments the purpose of a SessionHandle * Update comments a bit * Update src/transport/Session.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/Session.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/Session.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update Session.h Add explanation that even `move` was not fully intentionally allowed for SessionHandle. --------- Co-authored-by: Andrei Litvin <andreilitvin@google.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent b791b75 commit 242accd

File tree

1 file changed

+42
-0
lines changed

1 file changed

+42
-0
lines changed

src/transport/Session.h

+42
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,48 @@ class Session;
4545
* as pending removal but not actually removed yet. During this period, the handle is functional, but the underlying session
4646
* won't be able to be grabbed by any SessionHolder. SessionHandle->IsActiveSession can be used to check if the session is
4747
* active.
48+
*
49+
* A `SessionHandle` exists to guarantee that the object it points to will not be released while allowing passing
50+
* the handle as a reference, to not incur extra reference-counted Retain/Release calls.
51+
*
52+
* Example code that breaks assumptions (hard to reason about/maintain):
53+
*
54+
* void Process(ReferenceCountedHandle<Session> &handle);
55+
* class Foo {
56+
* ReferenceCountedHandle<Session> mSession;
57+
* void ResetSession() { mSession = createNewSession(); }
58+
* void DoProcessing() {
59+
* Process(mSession);
60+
* }
61+
*
62+
* static Foo& GetInstance();
63+
* };
64+
*
65+
* void Process(ReferenceCountedHandle<Session> &handle) {
66+
* Foo::GetInstance()->ResetSession(); // this changes the passed in handle
67+
* // trying to use "&handle" here may point to something else altogether.
68+
* }
69+
*
70+
* Above would be fixed if we would pass in the handles by value, however that adds extra code
71+
* to call Retain/Release every time. We could also by design say that passed in references will
72+
* not change, however historically the codebase is complex enough that this could not be ensured.
73+
*
74+
* The end result is the existence of SessionHandle which is NOT allowed to be held and it serves
75+
* as a marker of "Retain has been called and stays valid". The code above becomes:
76+
*
77+
* void Process(SessionHandle &handle);
78+
*
79+
* ....
80+
* void Foo::DoProcessing() {
81+
* SessionHandle handle(mSession); // retains the session and mSession can be independently changed
82+
* Process(&handle); // reference is now safe to use.
83+
* }
84+
*
85+
* To meet the requirements of "you should not store this", the Handle has additional restrictions
86+
* preventing modification (no assignment or copy constructor) and allows only move.
87+
* NOTE: `move` should likely also not be allowed, however we need to have the ability to
88+
* return such objects from method calls, so it is currently allowed.
89+
*
4890
*/
4991
class SessionHandle
5092
{

0 commit comments

Comments
 (0)