From e1f22f6bf4f9233e6a931925d710e4528533c67b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 2 May 2024 11:31:33 -0400 Subject: [PATCH 1/6] Explain in comments the purpose of a SessionHandle --- src/transport/Session.h | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/transport/Session.h b/src/transport/Session.h index d9840ec3ee33f1..c8532f8a65d83b 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -45,6 +45,43 @@ 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 guarante that the object it points to will not be released while allowing passing + * the handle as a reference, to not incur refeference-counted Retain/Release extra calls. + * + * Example code that breaks assumptions (hard to reason about/maintain): + * + * void Process(ReferenceCountedHandle &handle); + * class Foo { + * ReferenceCountedHandle mSession; + * void ResetSession() { mSession = createNewSession(); } + * void DoProcessing() { + * Process(mSession); + * } + * + * static Foo& GetInstance(); + * }; + * + * void Process(ReferenceCountedHandle &handle) { + * Foo::GetInstance()->ResetSession(); // this changes the passed in handle + * // trying to use "&handle" here may point to something else alltogether. + * } + * + * 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. + * } + * */ class SessionHandle { From a9a370b1dfb905a9ee38fcc96c231c98db1ae776 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 2 May 2024 11:34:54 -0400 Subject: [PATCH 2/6] Update comments a bit --- src/transport/Session.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/transport/Session.h b/src/transport/Session.h index c8532f8a65d83b..a450bd81cb7d26 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -82,6 +82,9 @@ class Session; * 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. + * */ class SessionHandle { From 6f68abc5df8cbb6126a897497625da8f10834d0b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 3 May 2024 12:08:54 -0400 Subject: [PATCH 3/6] Update src/transport/Session.h Co-authored-by: Boris Zbarsky --- src/transport/Session.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/Session.h b/src/transport/Session.h index a450bd81cb7d26..99efcead1e4787 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -46,7 +46,7 @@ class 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 guarante that the object it points to will not be released while allowing passing + * 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 refeference-counted Retain/Release extra calls. * * Example code that breaks assumptions (hard to reason about/maintain): From 6a50449b8bcde5b8eff0806b6fda01100b122216 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 3 May 2024 12:09:03 -0400 Subject: [PATCH 4/6] Update src/transport/Session.h Co-authored-by: Boris Zbarsky --- src/transport/Session.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/Session.h b/src/transport/Session.h index 99efcead1e4787..c1b5ddd191f4e8 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -47,7 +47,7 @@ class Session; * 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 refeference-counted Retain/Release extra calls. + * the handle as a reference, to not incur extra reference-counted Retain/Release calls. * * Example code that breaks assumptions (hard to reason about/maintain): * From 2f8f991b1d78366039584ed08fdee14043158001 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 3 May 2024 12:09:10 -0400 Subject: [PATCH 5/6] Update src/transport/Session.h Co-authored-by: Boris Zbarsky --- src/transport/Session.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/Session.h b/src/transport/Session.h index c1b5ddd191f4e8..30c9b6f8b9d536 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -64,7 +64,7 @@ class Session; * * void Process(ReferenceCountedHandle &handle) { * Foo::GetInstance()->ResetSession(); // this changes the passed in handle - * // trying to use "&handle" here may point to something else alltogether. + * // 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 From 210a171d1b8b381fa85c80d4ed04d60beba87f12 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 3 May 2024 12:11:35 -0400 Subject: [PATCH 6/6] Update Session.h Add explanation that even `move` was not fully intentionally allowed for SessionHandle. --- src/transport/Session.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/transport/Session.h b/src/transport/Session.h index 30c9b6f8b9d536..c18238c1bcacea 100644 --- a/src/transport/Session.h +++ b/src/transport/Session.h @@ -84,6 +84,8 @@ class Session; * * 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. + * 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