Improve ExchangeMessageDispatch #17521
Closed
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
anush-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
lazarkov,
LuDuda,
lzgrablic02,
mlepage-google,
mrjerryjohns,
msandstedt and
mspang
pullapprove
bot
requested review from
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
|
PR #17521: Size comparison from ff32d56 to 1efd09b Increases above 0.2%:
Increases (24 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, p6)
Decreases (32 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (32 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
msandstedt
approved these changes
Apr 20, 2022
| CHIP_ERROR err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get(), mExchangeId, IsInitiator(), | ||
| GetReliableMessageContext(), reliableTransmissionRequested, protocolId, msgType, | ||
| std::move(msgBuf)); | ||
| CHIP_ERROR err = ([&] { |
There was a problem hiding this comment.
This makes an already-large method much larger and quite hard to read/understand, what with the various "return" bits that return out of the lambda, not the overall method. What is the benefit of inlining this instead of having it as a separate function (possibly on ExchangeContext if we don't think it should be on the message dispatch)?
| @@ -288,6 +288,12 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const | |||
| } | |||
| } | |||
|
|
|||
| if (GetDispatchForDelegate(delegate).IsEncryptionRequired() != session->IsEncrypted()) | |||
| { | |||
| ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_UNSOLICITED_MESSAGE_HANDLER)); | |||
There was a problem hiding this comment.
That's not the error type we used to have here. Why is this the right type?
Also, this check will always fail if delegate is null, right? As long as we are moving this around, can we skip this check for the !delegate case where we're just trying to send a standalone ack? Or will something go terribly wrong if the session is not in fact encrypted and we try to send an ack via ApplicationExchangeDispatch?
|
I incline removing virtual bool MessagePermitted(uint16_t protocol, uint8_t type) = 0;
virtual bool IsEncryptionRequired() const { return true; }Only This approach is more memory efficient, less object to maintain, more robust and readable. @bzbarsky-apple any opinions ? |
|
Will split this PR into several small PRs |
|
I don't think it's just CASESession/PASESession that use IsEncryptionRequired... Past that, I don't have any particularly strong views about the way this ends up set up as long as we are doing equivalent checks with equivalent safety guarantees. |
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
There are a few TODO left over ExchangeManager and ExchangeContext
Change overview
ExchangeMessageDispatch::SendMessageandExchangeMessageDispatch::OnMessageReceived, as they are only used once, and they are non-virtualdispatch.IsEncryptionRequired()check at exchange creation, do not run the check on every message.Note:
mSession->RequireMRP()check.ExchangeContext::MatchExchangeis removed because it is inherited by session match.Testing
Passed unit-tests.