From 2dacc9dce3563ce54f5877628bd01fe361fce220 Mon Sep 17 00:00:00 2001
From: Zang MingJie <zealot0630@gmail.com>
Date: Sat, 18 Jun 2022 01:20:25 +0800
Subject: [PATCH] Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
---
 src/messaging/ExchangeMgr.cpp          |  2 +-
 src/messaging/ExchangeMgr.h            |  6 ++++--
 src/messaging/ReliableMessageContext.h |  7 ++++---
 src/transport/SecureSession.cpp        |  2 +-
 src/transport/SecureSession.h          | 10 ++++++----
 5 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp
index b9e133e75cdd53..05b70adec8adf9 100644
--- a/src/messaging/ExchangeMgr.cpp
+++ b/src/messaging/ExchangeMgr.cpp
@@ -379,7 +379,7 @@ void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * deleg
 void ExchangeManager::AbortExchangesForFabricExceptOne(FabricIndex fabricIndex, ExchangeContext * deferred)
 {
     mContextPool.ForEachActiveObject([&](auto * ec) {
-        if (ec->HasSessionHandle() && ec->GetSessionHandle()->GetPeer().GetFabricIndex() == fabricIndex)
+        if (ec->HasSessionHandle() && ec->GetSessionHandle()->GetFabricIndex() == fabricIndex)
         {
             if (ec == deferred)
                 ec->SetAutoReleaseSession();
diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h
index 69dbd80a79ff40..910dcb8ee81db3 100644
--- a/src/messaging/ExchangeMgr.h
+++ b/src/messaging/ExchangeMgr.h
@@ -183,8 +183,10 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate
      */
     void CloseAllContextsForDelegate(const ExchangeDelegate * delegate);
 
-    // This API is used by UpdateNOC command, to abort all exchanges except the given one, whose abort is deferred until UpdateNOC
-    // command finishing its work.
+    // This API is used by commands that need to shut down all existing exchanges on 
+    // a fabric but need to make sure the response to the command still goes out on the exchange
+    // the command came in on.  This API flags that one exchange to shut down its session
+    // when it's done.
     void AbortExchangesForFabricExceptOne(FabricIndex fabricIndex, ExchangeContext * deferred);
 
     SessionManager * GetSessionManager() const { return mSessionManager; }
diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h
index d4a89c73237e86..6e94da51f4a739 100644
--- a/src/messaging/ReliableMessageContext.h
+++ b/src/messaging/ReliableMessageContext.h
@@ -124,9 +124,10 @@ class ReliableMessageContext
     /// Determine whether this exchange is a EphemeralExchange for replying a StandaloneAck
     bool IsEphemeralExchange() const;
 
-    // For UpdateNOC command, I'm the last exchange, I must release the session when finishing my work.
+    // If SetAutoReleaseSession() is called, this exchange must be using a SecureSession, and should
+    // evict it when the exchange is done with all its work (including any MRP traffic).
     void SetAutoReleaseSession();
-    bool IsAutoReleaseSession();
+    bool ReleaseSessionOnDestruction();
 
     /**
      * Get the reliable message manager that corresponds to this reliable
@@ -169,7 +170,7 @@ class ReliableMessageContext
         /// When set, signifies that the exchange created sorely for replying a StandaloneAck
         kFlagEphemeralExchange = (1u << 9),
 
-        /// When set, automatically release the session when finishing its work. Used by UpdateNOC command
+        /// When set, automatically release the session when this exchange is destroyed.
         kFlagAutoReleaseSession = (1u << 10),
     };
 
diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp
index 6aa5cc044a868f..e63d2d861e4ce1 100644
--- a/src/transport/SecureSession.cpp
+++ b/src/transport/SecureSession.cpp
@@ -61,7 +61,7 @@ void SecureSession::MarkForInactive()
         VerifyOrDie(false);
         return;
     case State::kActive:
-        // By setting this state, IsActiveSession() will return false, which prevent creating new exchanges.
+        // By setting this state, IsActiveSession() will return false, which prevents creating new exchanges.
         mState = State::kInactive;
         return;
     case State::kInactive:
diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h
index 9978918929a1d2..6ce8202c65b2d3 100644
--- a/src/transport/SecureSession.h
+++ b/src/transport/SecureSession.h
@@ -144,7 +144,7 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
     /// @brief Mark as pending removal, all holders to this session will be cleared, and disallow future grab
     void MarkForRemoval();
 
-    // Used by UpdateNOC command to prevent any new exchange created on the session.
+    // Used to prevent any new exchange created on the session while the existing exchanges finish their work.
     void MarkForInactive();
 
     Session::SessionType GetSessionType() const override { return Session::SessionType::kSecure; }
@@ -253,9 +253,11 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
         // session object will be released automatically.
         kPendingRemoval = 3,
 
-        // For UpdateNOC command, the session still functional, but it can't
-        // yield any new exchanges, the last exchange running UpdateNOC command
-        // will close the session soon.
+        // The session is still functional, but it can't yield any new exchanges,
+        // This is meant to be used in conjunction with 
+        // ExchangeManager::AbortExchangesForFabricExceptOne, with the one
+        // exceptional exchange handling moving this session out of this state when
+        // it finishes whatever it needs the session for.
         kInactive = 4,
     };