-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Make exchanges handle closing on message receipt more automatically. #8050
Make exchanges handle closing on message receipt more automatically. #8050
Conversation
Figure out which channel to use for the received messageconnectedhomeip/src/messaging/ExchangeMgr.cpp Lines 279 to 289 in ba265d1
This comment was generated by todo based on a
|
ba265d1
to
8187c4d
Compare
@@ -71,6 +71,8 @@ CHIP_ERROR CommandHandler::SendCommandResponse() | |||
MoveToState(CommandState::Sending); | |||
|
|||
exit: | |||
// Keep Shutdown() from double-closing our exchange. | |||
mpExchangeCtx = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exchange is auto closed on message receipt, how this exchange is closed after sending response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we call SendCommandResponse
before we return from OnInvokeCommandRequest
and hence before the auto-closing happens. If that changes, we would switch to calling WillSendMessage
under OnInvokeCommandRequest
and then close in SendCommandResponse
(I plan to make this pretty automatic by closing under SendMessage
in a followup PR, fwiw).
So if this line were not there, Shutdown
would call Abort
on the exchange and then the exchange itself would call Close
on itself, messing up the reference counting.
Again, this will become much cleaner when we no longer need any explicit closing from outside the exchange, just clearly letting it know what our application state is. This is the first step to getting there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we use setting mpExchangeCtx = nullptr to prevent double close and assume the exchange will be closed under hood.
Since we do have a response to send, how about calling WillSendMessage in OnInvokeCommandRequest before SendCommandResponse and leave to Shutdown to close the exchange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn't really change anything, because the sequence of events would then presumably be:
ExchangeContext::HandleMessage
is entered.OnInvokeCommandRequest
is called.WillSendMessage
is called.SendCommandResponse
is called and clears the "will send message" flag: the message that was promised has now been sent.- The
Shutdown
call inSendCommandResponse
aborts the exchange. - The auto-close code is reached.
That is, WillSendMessage
only makes sense if you plan to go async with your message sending.
That said, the fact that the auto-close code now checks whether the exchange is already closed (something I needed to add to deal with the way we hand acks to already-closed exchanges) means this pattern, or indeed not calling WillSendMessage
and not nulling out mpExchangeCtx
would be ok. It feels to me that it would be a little confusing to have two different callsites that are both "responsible for closing the exchange", one of which just happens to not do anything if the other one has happened. But I'd be OK with it as a temporary state of affairs. Again, the goal is to move to a world where there is no manual closing of exchanges at all, just calls about application-level desires that then lead the exchange to be closed once it's no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the result will be the same. The tricky part now is the auto-close could be triggered in any case, the application needs to clearly figure out if the auto-close will be reached later and take action to prevent double close if needed. Such as by setting exchange to nullptr.
Do you think if we can make life easier if we enforce the following rules?
- The exchange will always be auto-closed on message receipt if there is no more message needed to be sent.
- If there is more message to be sent, application always set WillSendMessage flag, and manually close the exchange after extra message is sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My long-term plan (of which this PR is the first step) is to have something like the following rules:
- An exchange can be in one of three states: waiting for send, waiting for receive, or closed.
- Calling SendMessage on the exchange transitions to "waiting for receive" if the "wait for response" flag is set, or to closed otherwise.
- Calling HandleMessage on the exchange can only happen in the "waiting for receive" state and can land it in any of the three states, corresponding to the delegate calling WillSendMessage, calling SendMessage with the "wait for response" flag, or to the delegate calling neither. Some exceptions here for things that are there just for CRMP (i.e. HandleMessage calls for acks and duplicates), which we might perhaps want to factor out into a different API, not HandleMessage.
- Timing out an exchange that is in "waiting for receive" state has the same general behavior as HandleMessage.
- Exchanges are not reference counted. An exchange waiting for send is owned by the application. An exchange waiting for receive or closed is owned by the CHIP stack.
- Ideally you can't have a reference to an exchange you don't own. This part might require some handle classes that I have not thought completely through yet.
But that's long-term. For purposes of this PR the two rules you describe are pretty much what we are aiming for, except that in addition to doing WillSendMessage and then manually closing you can just send a message that expects a response, which is one step closer to my goal rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those rules sound good to me, I am ok we only handle the auto-close on receipt with this PR as the first steps to achieve those goals.
@@ -190,7 +190,6 @@ CHIP_ERROR InteractionModelEngine::OnUnknownMsgType(Messaging::ExchangeContext * | |||
// err = SendStatusReport(ec, kChipProfile_Common, kStatus_UnsupportedMessage); | |||
// SuccessOrExit(err); | |||
|
|||
apExchangeContext->Close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since an unknown message is received here, how we know the exchange is auto closed in this case since we don't know if kFlagWillSendMessage flag is set for an unknown message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. We're in an unsolicited message handler. The only way we land in this code is if an unsolicited message came in off the network. This is therefore a brand-new exchange that was just created. We're not telling anyone anything about this exchange; just silently closing it, before this PR. After this PR we continue silently closing it, since none of the conditions that would cause it to not close apply in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OnMessageReceived is used as both unsolicited message handler and received message handler. why the only way to land in this code is an unsolicited message came in off the network?
-
Suppose InvokeCommandRequest is first received, and an new exchange is created by ExchangeMgr and handle over InvokeCommandRequest to OnMessageReceived, then we should fall under OnInvokeCommandRequest, right?
-
Suppose the other end send an message other than InvokeCommandRequest/ReadRequest/WriteRequest over an existing exchange, we could also fall under OnUnknownMsgType, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OnMessageReceived is used as both unsolicited message handler and received message handler.
Hmm... I thought for some reason that IMEngine reset the delegate to the object it was handing off to, but it clearly does not....
But the reason I had thought that is that what the code actually does is totally unsafe. Let's say that we get a Read Request message. We allocate a ReadHandler, call OnReadRequest on it, that goes async waiting to send a response on the exchange (this is with or without my changes; the difference is that with my changes we call WillSendMessage
when we go async).
If now an unknown message comes in on the same exchange, the code before my change will close the exchange, and now the ReadHandler has a dangling pointer to a deleted exchange, and will crash when it tries to send the read response. This is exactly the sort of thing I am trying to fix by switching from manual exchange management to telling the exchange what you plan to do with it....
Back to your specific scenario: That one is actually ok with or without my changes, for the moment, but only because OnInvokeCommandRequest
synchronously replies and closes the exchange. That means that (outside of the loopback setup in tests) the second message, which would go to OnUnknownMsgType
, never gets there, because the exchange is already closed, its delegate nulled out, and hence we can't land in OnMessageReceived
. Once commands start doing async work it looks more like the Read Request case above.
Now the argument might be that if we got a message the spec did not intend on an exchange we should close it. But that's also true if an exchange that started with an Invoke Request suddenly gets a Read Request. That is not an expected message on that exchange, and it should be treated like any other random unexpected message.
Probably the simplest way to make that work is in fact to make ReadHandler/CommandHandler/WriteHandler into ExchangeDelegates and have them take over all message-handling on the exchange until the decide they are done with it and close it. I am happy to do that as a followup to this PR, or as prereq if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
@@ -82,6 +82,8 @@ CHIP_ERROR ReadHandler::OnReadRequest(Messaging::ExchangeContext * apExchangeCon | |||
if (err != CHIP_NO_ERROR) | |||
{ | |||
ChipLogFunctError(err); | |||
// Keep Shutdown() from double-closing our exchange. | |||
mpExchangeCtx = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where we close the mpExchangeCtx after ProcessReadRequest? I see we called mpExchangeCtx->WillSendMessage() within ProcessReadRequest to keep this exchange open
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProcessReadRequest
only calls WillSendMessage
if it's returning success. So there are two cases:
- ProcessReadRequest returns success. Then it called
WillSendMessage
and it's responsible for closing the exchange later. In that case we have err == CHIP_NO_ERROR here and we do NOT null outmpExchangeCtx
, so we can close it later. - ProcessReadRequest returns failure. In that case it has not called
WillSendMessage
, the exchange will auto-close, we enter this block, and we need to keepShutdown
from doing an extra close on the exchange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this logic require us not add any logic within ProcessReadRequest to return error after following code, right?
if (mpExchangeCtx)
{
mpExchangeCtx->WillSendMessage();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. I will add a comment to that effect.
Again, this is a temporary state of things while we have these manual Close/Abort calls. Which, again, I am aiming to get rid of altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ProcessReadRequest returns failure, I think the auto-close is reached after Shutdown, right?
err = ProcessReadRequest(std::move(aPayload));
if (err != CHIP_NO_ERROR)
{
ChipLogFunctError(err);
// Keep Shutdown() from double-closing our exchange.
mpExchangeCtx = nullptr;
Shutdown();
}
Do we set mpExchangeCtx = nullptr before or after the auto-close in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ProcessReadRequest
returns failure, then:
- We never call
WillSendMessage
. - We null out
mpExchangeCtx
before the auto-close. - We call
Shutdown
before the auto-close, but that does not touch the exchange becausempExchangeCtx
is null. - The auto-close happens when the stack unwinds, after
ReadHandler::OnReadRequest
returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, the stack unwind will make sure the exchange is not leaked after we nullify the mpExchangeCtx
@@ -74,6 +74,8 @@ CHIP_ERROR WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeC | |||
|
|||
exit: | |||
ChipLogFunctError(err); | |||
// Keep Shutdown() from double-closing our exchange. | |||
mpExchangeCtx = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above, where the exchange is closed after ProcessWriteRequest, shall we call mpExchangeCtx->WillSendMessage(); in ProcessWriteRequest as ProcessReadRequest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProcessWriteRequest does not do any async work right now. We send the response immediately after calling ProcessWriteRequest
and want to close the exchange right after that.
We could call WillSendMessage
in ProcessWriteResponse
. That flag would just get cleared by the send that SendWriteResponse
does. Unless that function fails to reach the send call, in which case we'd need to close manually. Doable, but a bit more complicated than what we have now. If we start doing async work in WriteHandler, we should add the WillSendMessage
at that point.
8187c4d
to
675c69d
Compare
@yufengwangca Note that I just made a change to the closing logic, replacing |
Size increase report for "gn_qpg6100-example-build" from baddb92
Full report output
|
// Also don't close if there's an outer HandleMessage invocation. It'll | ||
// deal with the closing. | ||
if (!mFlags.Has(Flags::kFlagClosed) && !mFlags.Has(Flags::kFlagWillSendMessage) && !IsResponseExpected() && | ||
(!isStandaloneAck || (mDelegate == nullptr)) && (!isDuplicate || (mDelegate == nullptr)) && !alreadyHandlingMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain more bout these two conditions? (!isStandaloneAck || (mDelegate == nullptr)) && (!isDuplicate || (mDelegate == nullptr))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I tried to explain in the comments in the code; if those are not clear enough I need to work on that and would appreciate wording suggestions.
Those cases are meant to detect the case when we create an exchange just to ack a message that does not have a UMH. In that situation we will create an exchange with a null delegate and call HandleMessage on it. In that situation, we want to close the exchange even if the message is an ack or a duplicate.
We could simplify things a bit perhaps by factoring out the "send an ack" codepath so that it can be invoked directly in the "just send an ack" case instead of conflating it with the generic HandleMessage codepath. That would allow simpler logic here, at the cost of resurrecting the "send ack and close" logic in the exchange manager.
Now maybe we could have a situation where someone creates an exchange with a null delegate, sends a message on it, doesn't call WillSendMessage
to indicate the intent to send more messages and doesn't say a response is expected (and such a response would not be seen anyway, because of the null delegate). But in such a situation, I'd think closing the exchange on receipt of the ack is in fact the right course of action: semantically there is no plan to send more messages on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand we don't close the exchange on receipt of the standalong ack, since we expect the exchange will be closed on receipt of the response message later.
Why we don't close the exchange on duplicate message? who are going to close the exchange after we switch to auto-close pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to close the exchange on application-level events like "last message of conversation sent" or "last message of conversation received". The exchange knows when messages are sent or received; the app needs to indicate whether it's the "last" one. For receipt it indicates this by flagging whether it plans to send a response, or just sending a response. For send it does this via the "response expected" flag.
Duplicate message receipt is not an application-level event. The only reason duplicates are even delivered to an exchange is so that we can generate a MRP ack for them.
One thing we could consider doing to simplify the logic here is to treat duplicates like we treat unsolicited messages with no handler and just create a separate exchange to ack them, even if they match a missing exchange. But I'd rather do that sort of thing in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That make sense to me, only the application who understand the protocol context has the knowledge of the last message of the conversation. This concept does not change with this PR, the only difference is the application needs to take actions to explicitly indicate to extend the exchange, otherwise the exchange will be closed automatically by itself.
I understand we don't need to deliver duplicate message to application and only do MRP ack within exchange, my question is who finally close the exchange after MRP ack for duplicate message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my question is who finally close the exchange after MRP ack for duplicate message?
Ah, I see. There are two cases here:
- The exchange is being used for an actual application-level conversation. In that case, the exchange will be closed in the normal course of that conversation.
- The exchange was created just to ack the duplicate. In that case, it will have a null delegate and the auto-closing will happen; hence the form of the
(!isDuplicate || (mDelegate == nullptr))
condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acked
675c69d
to
463f135
Compare
Size increase report for "esp32-example-build" from 2478e02
Full report output
|
Size increase report for "nrfconnect-example-build" from 2478e02
Full report output
|
463f135
to
39a6309
Compare
The idea is that the cases that want to keep the exchange open should do so explicitly and in all other cases the exchange should close. This helps avoid exchange leaks and makes it much clearer when an exchange is being kept open.
39a6309
to
970ee1b
Compare
Size increase report for "gn_qpg-example-build" from 491f5f4
Full report output
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarified offline, this PR currently only handle the auto-close on message receipt, the exchang e state handling and auto-close on sending path will be done in the following PRs.
@andy31415 @mspang @Damian-Nordic @jepenven-silabs @LuDuda @saurabhst @msandstedt @jmartinez-silabs please take a look? |
// This needs to be done before the Reset() call, because Reset() aborts mpExchangeCtx if its not null. | ||
mpExchangeCtx->Close(); | ||
// Null out mpExchangeCtx, so our Shutdown() call below won't try to abort | ||
// it and fail to send an ack for the message we just received. | ||
mpExchangeCtx = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem less error prone than the previous behavior. It just seems different. What if we don't set mpExchangeCtx to NULL in the callee? If being called by the ExchangeDelegate::OnMessageReceived
interface means the exchange manager now owns the exchange context, why don't we simply make that happen automatically?
I think this could be done by moving mpExchangeCtx to ExchangeDelegate
as a public member, or moving it to ExchangeDelegate
as protected member and then making ExchangeDelegate
a friend of ExchangeManager
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msandstedt See #8050 (comment) for the general goal here; this is step 1.
What if we don't set mpExchangeCtx to NULL in the callee?
Then things are bad, yes, after this PR. The goal here is to not have an mpExchangeCtx
in the callee in this case at all, but it'll take a few steps.
I think this could be done by moving mpExchangeCtx to ExchangeDelegate as a public member,
Note that CommandSender isn't even an ExchangeDelegate, so that would not work for this specific case, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the big reason not to do what I'm suggesting is that this would restrict implementers of ExchangeDelegate
to only hold a single exchange context. Still, it really seems like the right solution here is going to be something where everything is automatic.
Can we change the relevant IM interfaces to take & mpExchangeCtx
so they, or something down the line, sets it null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. Makes sense. This is just the first step to get where we want to go, and is an improvement.
…roject-chip#8050) The idea is that the cases that want to keep the exchange open should do so explicitly and in all other cases the exchange should close. This helps avoid exchange leaks and makes it much clearer when an exchange is being kept open.
The idea is that the cases that want to keep the exchange open should
do so explicitly and in all other cases the exchange should close.
This helps avoid exchange leaks and makes it much clearer when an
exchange is being kept open.
Problem
Closing exchanges manually is very fragile. This is step 1 of having it be a lot more automatic, with app-level context being used to decide whether to close.
Change overview
Exchanges auto-close on message receipt unless the app either sends a message expecting a response or indicates it plans to do so.
Testing
No behavior change (ideally) if all of our consumers are handling this correctly. This code is exercised by a variety of unit tests, so I have decent confidence in it.