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

Addition of SessionSharedPtr #18539

Closed
Closed
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions src/transport/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ static_library("transport") {
"SessionManager.h",
"SessionMessageCounter.h",
"SessionMessageDelegate.h",
"SessionSharedPtr.h",
"TransportMgr.h",
"TransportMgrBase.cpp",
"TransportMgrBase.h",
Expand Down
20 changes: 12 additions & 8 deletions src/transport/SessionHandle.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
#pragma once

#include <access/SubjectDescriptor.h>
#include <lib/core/Optional.h>
#include <lib/support/ReferenceCountedHandle.h>
#include <transport/SessionSharedPtr.h>

namespace chip {

Expand All @@ -29,27 +31,29 @@ class Session;
class SessionHolder;

/** @brief
* Non-copyable session reference. All SessionHandles are created within SessionManager. SessionHandle is not
* reference *counted, hence it is not allowed to store SessionHandle anywhere except for function arguments and
* return values. SessionHandle is short-lived as it is only available as stack variable, so it is never dangling. */
class SessionHandle
* A non-copyable version of SessionSharedPtr that is always guaranteed to point to a valid Session. This is always
* created within SessionManager.
*
* Since this is a strong reference, this should be short-lived and only used as a stack variable.
*/
class SessionHandle : protected SessionSharedPtr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
public:
SessionHandle(Transport::Session & session) : mSession(session) {}
SessionHandle(Transport::Session & session) : SessionSharedPtr(session) {}
~SessionHandle() {}

SessionHandle(const SessionHandle &) = delete;
SessionHandle operator=(const SessionHandle &) = delete;
SessionHandle(SessionHandle &&) = default;
SessionHandle & operator=(SessionHandle &&) = delete;

bool operator==(const SessionHandle & that) const { return &mSession.Get() == &that.mSession.Get(); }
bool operator==(const SessionHandle & that) const { return Get() == that.Get(); }

Transport::Session * operator->() const { return mSession.operator->(); }
Transport::Session * operator->() const { return SessionSharedPtr::operator->(); }

private:
// TODO: Remove this once SessionHolder pivots to truly being a weak reference (#18399).
friend class SessionHolder;
ReferenceCountedHandle<Transport::Session> mSession;
};

} // namespace chip
2 changes: 1 addition & 1 deletion src/transport/SessionHolder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ SessionHolder & SessionHolder::operator=(SessionHolder && that)
void SessionHolder::Grab(const SessionHandle & session)
{
Release();
mSession.Emplace(session.mSession);
mSession.Emplace(session.mSession.Value());
session->AddHolder(*this);
}

Expand Down
3 changes: 2 additions & 1 deletion src/transport/SessionHolder.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ class SessionHolder : public SessionDelegate, public IntrusiveListNodeBase

bool Contains(const SessionHandle & session) const
{
return mSession.HasValue() && &mSession.Value().Get() == &session.mSession.Get();
auto ourSession = ToOptional();
return ourSession.HasValue() && ourSession.Value() == session;
}

void Grab(const SessionHandle & session);
Expand Down
105 changes: 105 additions & 0 deletions src/transport/SessionSharedPtr.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
*
* Copyright (c) 2021 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#pragma once

#include <access/SubjectDescriptor.h>
#include <lib/core/Optional.h>
#include <lib/support/ReferenceCountedHandle.h>

namespace chip {

namespace Transport {
class Session;
} // namespace Transport

class SessionHolder;

/** @brief
* A shared_ptr like smart pointer to manage the lifetime of a Session object. Like a shared_ptr,
* this object can start out not tracking any Session and be attached to a Session there-after.
* The underlying Session is guaranteed to remain active and resident until all references to it from SessionSharedPtr
* instances in the system have gone away, at which point it invokes its custom destructor.
*
* Just because a Session is refcounted does not mean it actually gets destroyed upon reaching a count of 0.
* UnauthenticatedSession and SecureSession have different logic that gets invoked when the count hits 0.
*
* This should really only be used during session setup by the entity setting up the session.
* Once setup, the session should transfer ownership to the SessionManager at which point,
* all clients in the system should only be holding SessionWeakPtrs (SessionWeakPtr doesn't exist yet, but once
* #18399 is complete, SessionHolder will become SessionWeakPtr).
*
* This is copy-constructible.
*/
class SessionSharedPtr
{
public:
SessionSharedPtr() {}
SessionSharedPtr(Transport::Session & session) { mSession.Emplace(session); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


SessionSharedPtr(const SessionSharedPtr & session) { Grab(session); }

/*
* Add ourselves as a shared owner on the passed in session (if it points to
* a valid session). The passed in
* session object can now be free'ed safely without impacting the underlying
* Session.
*/
void operator=(const SessionSharedPtr & session) { Grab(session); }

/*
* If we're currently pointing to a valid session, remove ourselves
* as a shared owner of that session. If there are no more shared owners
* on that session, that session MAY be reclaimed.
*/
void Release() { mSession.ClearValue(); }

/*
* Assume shared ownership of the provided session.
*/
void Grab(Transport::Session & session) { mSession.Emplace(session); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


Transport::Session * operator->() const { return Get(); }

protected:
Transport::Session * Get() const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be private.

Add operator==(const SessionSharedPtr &) API, to help implementing SessionHandle::operator==

{
if (mSession.HasValue())
{
return &mSession.Value().Get();
}

return nullptr;
}

private:
void Grab(const SessionSharedPtr & session)
{
auto * underlyingSession = session.Get();
if (underlyingSession)
{
mSession.Emplace(*underlyingSession);
}
}

Optional<ReferenceCountedHandle<Transport::Session>> mSession;

// TODO: Remove this once SessionHolder pivots to truly being a weak reference (#18399).
friend class SessionHolder;
};

} // namespace chip