Fix some nonsensical code/comments in CommandSender/CommandHandler. (#…

…12874)

#12716 copies some
bits without modifying them, but they don't make sense in the new
places.  Some of the code can go, and the comments need adjusting.

src/app/CommandHandler.cpp

@@ -149,18 +149,11 @@ void CommandHandler::Close()
     VerifyOrDieWithMsg(mPendingWork == 0, DataManagement, "CommandHandler::Close() called with %zu unfinished async work items",
                        mPendingWork);
 
-    //
-    // Shortly after this call to close and when handling an inbound message, it's entirely possible
-    // for this object (courtesy of its derived class) to be destroyed
-    // *before* the call unwinds all the way back to ExchangeContext::HandleMessage.
-    //
-    // As part of tearing down the exchange, there is logic there to invoke the delegate to notify
-    // it of impending closure - which is this object, which just got destroyed!
-    //
-    // So prevent a use-after-free, set delegate to null.
+    // OnDone below can destroy us before we unwind all the way back into the
+    // exchange code and it tries to close itself.  Make sure that it doesn't
+    // try to notify us that it's closing, since we will be dead.
     //
     // For more details, see #10344.
-    //
     if (mpExchangeCtx != nullptr)
     {
         mpExchangeCtx->SetDelegate(nullptr);
@@ -494,9 +487,6 @@ const char * CommandHandler::GetStateStr() const
     case State::AddedCommand:
         return "AddedCommand";
 
-    case State::AwaitingTimedStatus:
-        return "AwaitingTimedStatus";
-
     case State::CommandSent:
         return "CommandSent";
 
@@ -522,9 +512,9 @@ void CommandHandler::Abort()
     //
     if (mpExchangeCtx != nullptr)
     {
-        // We (or more precisely our subclass) might be a delegate for this
-        // exchange, and we don't want the OnExchangeClosing notification in
-        // that case.  Null out the delegate to avoid that.
+        // We might be a delegate for this exchange, and we don't want the
+        // OnExchangeClosing notification in that case.  Null out the delegate
+        // to avoid that.
         //
         // TODO: This makes all sorts of assumptions about what the delegate is
         // (notice the "might" above!) that might not hold in practice.  We

src/app/CommandHandler.h

@@ -185,17 +185,6 @@ class CommandHandler
      */
     bool IsTimedInvoke() const { return mTimedRequest; }
 
-    enum class CommandState
-    {
-        Idle,                ///< Default state that the object starts out in, where no work has commenced
-        AddingCommand,       ///< In the process of adding a command.
-        AddedCommand,        ///< A command has been completely encoded and is awaiting transmission.
-        AwaitingTimedStatus, ///< Sent a Timed Request and waiting for response.
-        CommandSent,         ///< The command has been sent successfully.
-        ResponseReceived,    ///< Received a response to our invoke and request and processing the response.
-        AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application.
-    };
-
     /*
      * This forcibly closes the exchange context if a valid one is pointed to. Such a situation does
      * not arise during normal message processing flows that all normally call Close() above. This can only
@@ -222,7 +211,6 @@ class CommandHandler
         Idle,                ///< Default state that the object starts out in, where no work has commenced
         AddingCommand,       ///< In the process of adding a command.
         AddedCommand,        ///< A command has been completely encoded and is awaiting transmission.
-        AwaitingTimedStatus, ///< Sent a Timed Request and waiting for response.
         CommandSent,         ///< The command has been sent successfully.
         AwaitingDestruction, ///< The object has completed its work and is awaiting destruction by the application.
     };

src/app/CommandSender.cpp

@@ -206,18 +206,11 @@ void CommandSender::Close()
     mTimedRequest     = false;
     MoveToState(State::AwaitingDestruction);
 
-    //
-    // Shortly after this call to close and when handling an inbound message, it's entirely possible
-    // for this object (courtesy of its derived class) to be destroyed
-    // *before* the call unwinds all the way back to ExchangeContext::HandleMessage.
-    //
-    // As part of tearing down the exchange, there is logic there to invoke the delegate to notify
-    // it of impending closure - which is this object, which just got destroyed!
-    //
-    // So prevent a use-after-free, set delegate to null.
+    // OnDone below can destroy us before we unwind all the way back into the
+    // exchange code and it tries to close itself.  Make sure that it doesn't
+    // try to notify us that it's closing, since we will be dead.
     //
     // For more details, see #10344.
-    //
     if (mpExchangeCtx != nullptr)
     {
         mpExchangeCtx->SetDelegate(nullptr);
@@ -445,9 +438,9 @@ void CommandSender::Abort()
     //
     if (mpExchangeCtx != nullptr)
     {
-        // We (or more precisely our subclass) might be a delegate for this
-        // exchange, and we don't want the OnExchangeClosing notification in
-        // that case.  Null out the delegate to avoid that.
+        // We might be a delegate for this exchange, and we don't want the
+        // OnExchangeClosing notification in that case.  Null out the delegate
+        // to avoid that.
         //
         // TODO: This makes all sorts of assumptions about what the delegate is
         // (notice the "might" above!) that might not hold in practice.  We