Addition of SessionSharedPtr
#18539
Closed
Addition of SessionSharedPtr
#18539
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
msandstedt and
mspang
pullapprove
bot
requested review from
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
msandstedt,
mspang,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
mrjerryjohns
force-pushed
the
session-shared-ptr
branch
from
de33323 to
101900c
Compare
|
PR #18539: Size comparison from a777a80 to 101900c Increases above 0.2%:
Increases (31 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (31 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
mrjerryjohns
force-pushed
the
session-shared-ptr
branch
from
101900c to
63a2208
Compare
kghost
reviewed
May 19, 2022
| { | ||
| public: | ||
| SessionSharedPtr() {} | ||
| SessionSharedPtr(Transport::Session & session) { mSession.Emplace(session); } |
There was a problem hiding this comment.
There should be no naked session object around, the only usage of this function is called by the constructor of SessionHandle we should make this API protected.
There was a problem hiding this comment.
We had some difficutly writing unit tests whenever we have too strict of friend class coupling.
I believe APIs/classes should be able to function as a stand alone without demanding fried class usage.
| /* | ||
| * Assume shared ownership of the provided session. | ||
| */ | ||
| void Grab(Transport::Session & session) { mSession.Emplace(session); } |
There was a problem hiding this comment.
There should be no naked session object around, the only usage of this function is inside PairingSession to hold the unauthenticated session, where it is grabbing a SessionHandle.
void Grab(const Transport::Session & SessionHandle)
| * | ||
| * Since this is a strong reference, this should be short-lived and only used as a stack variable. | ||
| */ | ||
| class SessionHandle : protected SessionSharedPtr |
There was a problem hiding this comment.
SessionHandle and SessionSharedPtr are basically same thing exposing different API. SessionHandle is immutable, SessionSharedPtr is mutable. Instead of let SessionHandle derive from SessionSharedPtr, I suggest we do it in the opposite way, use Optional<SessionHandle> as a field of SessionSharedPtr.
In such implemetation, the SessionHandle is a word smaller, and all APIs of SessionSharedPtr should target SessionHandle not naked session.
The only problem is that SessionHandle is should be copy-able by SessionSharedPtr, which we can resolve by using a private copy constructor and a friend declare.
The goal of this class is not non-copyable, but prevent from creating this object in heap, to ensure that the object is ephemeral. Generally C++ can't do that, it is just a coding convention.
| Transport::Session * operator->() const { return Get(); } | ||
|
|
||
| protected: | ||
| Transport::Session * Get() const |
There was a problem hiding this comment.
This should be private.
Add operator==(const SessionSharedPtr &) API, to help implementing SessionHandle::operator==
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
SessionHolderis used both as a strong, shared_ptr like smart-ptr object (it has ref-counting) as well as a weak_ptr like object (the underlying reference can be removed by SessionManager when evicting a session).This is confusing and makes it incongruous with similar smart-ptr constructs in STL.
Solution
Creates a new
SessionSharedPtras per #18399 that now plays the part of a strong, shared_ptr like construct. This sets the stage for us to pivotSessionHolderto a pure, weak_ptr like construct.Entities like
PairingSession,DeviceControllerthat need to momentarily own a session while it is being setup are already pivoting to using aSessionSharedPtrlike model in #18397.Once this PR merges, we can fully shift them over.