Fix ExchangeContext leaks #21846
Merged
Fix ExchangeContext leaks #21846
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
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
msandstedt,
mspang and
robszewczyk
mrjerryjohns
force-pushed
the
fix-exchange-closing
branch
2 times, most recently
from
4f0b8a2 to
c77a2cf
Compare
This makes some key fixes to the management of the ExchangeContext, specifically around how the WillSendMessage flag is set/cleared, as well as dealing with clean-up of the exchange in OnSessionReleased(). It also fixes some key bugs in ExchangeHolder around failure to release the ref on the exchange in certain scenarios as outlined in project-chip#21544 and project-chip#21632. Notably, the following fixes have been made to ExchangeContext: - Setting the WillSendMessage flag immediately in SendMessage and only clearing the WillSendMessage flag on actually having successfully sent a message in SendMessage. Otherwise, the holder is not able to infer that the ownership still resides with it on a SendMessage failure. - Not clearing the WillSendMessage flag in OnSessionReleased. This ensures the holder again can infer that it still has the ref and needs to give it up when OnExchangeClosing is called there-after. - Avoiding a double call of DoClose by checking if it is already closed. - Cleaning up OnSessionReleased logic to now correctly call Abort() in both the case where we're waiting for a response AND we're in the middle of received message dispatch (i.e we're in the middle of OnMessageReceived). This is a pre-existing bug where the closure of the exchange resulted in a leak due to MessageHandled() not quite cleaning up the EC later when the call unwinds. Tests: TestExchangeHolder has been buffed up to now exhaustively test permutations of send failure, session closure (both before and after message transmission) and calling WillSendMessage at all points along a 3 message transmission.
mrjerryjohns
force-pushed
the
fix-exchange-closing
branch
from
c77a2cf to
5fd3b71
Compare
mrjerryjohns
commented
Aug 12, 2022
This was
linked to
issues
Aug 12, 2022
|
PR #21846: Size comparison from afd4ab7 to 5fd3b71 Increases above 0.2%:
Increases (39 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (39 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, p6, telink)
|
bzbarsky-apple
approved these changes
Aug 12, 2022
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
andy31415
reviewed
Aug 17, 2022
|
PR #21846: Size comparison from 7870328 to d4bb284 Increases above 0.2%:
Increases (39 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, psoc6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (39 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, psoc6, telink)
|
…onditional in TestCommandInteraction
mrjerryjohns
changed the title
|
PR #21846: Size comparison from 3d7cc78 to 4146763 Increases (40 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, psoc6, telink)
Decreases (10 builds for cc13x2_26x2, linux)
Full report (41 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, psoc6, telink)
|
bzbarsky-apple
approved these changes
Aug 26, 2022
|
PR #21846: Size comparison from 3d7cc78 to 1c72c8e Increases above 0.2%:
Increases (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
Decreases (21 builds for cc13x2_26x2, esp32, linux, nrfconnect, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
yunhanw-google
approved these changes
Aug 29, 2022
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
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.
This makes some key fixes to the management of the
ExchangeContext, specifically around how theWillSendMessageflag is set/cleared, as well as dealing with clean-up of the exchange inOnSessionReleased(). It also fixes some key bugs in ExchangeHolder around failure to release the ref on the exchange in certain scenarios as outlined in bugs #21544 and #21632.Notably, the following fixes have been made to
ExchangeContext:Setting the
WillSendMessageflag immediately inSendMessageand only clearing the WillSendMessage flag on actually having successfully sent a message inSendMessage. Otherwise, the holder is not able to infer that the ownership still resides with it on aSendMessagefailure.Not clearing the
WillSendMessageflag in OnSessionReleased. This ensures the holder again can infer that it still has the ref and needs to give it up whenOnExchangeClosingis called there-after.Avoiding a double call of
DoCloseby checking if it is already closed.Cleaning up
OnSessionReleasedlogic to now correctly callAbort()inboth the case where we're waiting for a response AND we're in the
middle of received message dispatch (i.e we're in the middle of
OnMessageReceived). This is a pre-existing bug where the closure of the exchange resulted in a leak due toMessageHandled()not quite cleaning up the EC later when the call unwinds.Tests:
TestExchangeHolderhas been buffed up to now exhaustively test permutations of send failure, session closure (both before and after message transmission) and calling WillSendMessage at all points along a3 message transmission.