Simplify ExchangeMessageDispatch, preventing it from dangling #12794
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,
austinh0,
balducci-apple,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
harimau-qirex,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
LuDuda,
lzgrablic02,
mlepage-google,
msandstedt,
pan-apple,
robszewczyk,
sagar-apple,
saurabhst,
selissia and
tcarmelveilleux
kghost
force-pushed
the
exchange-dispatch
branch
from
5cb0a6b to
5a44abb
Compare
|
PR #12794: Size comparison from 7ae1cc9 to 5a44abb Increases above 0.2%:
Increases (8 builds for linux, mbed, p6)
Decreases (29 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
kghost
force-pushed
the
exchange-dispatch
branch
from
5a44abb to
3a661b2
Compare
|
PR #12794: Size comparison from bf7feea to 3a661b2 Increases above 0.2%:
Increases (21 builds for esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
Decreases (27 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
approved these changes
Dec 22, 2021
|
PR #12794: Size comparison from 6e035c8 to 7b4fbe8 Increases above 0.2%:
Increases (21 builds for esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
Decreases (27 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
This was referenced Jan 7, 2022
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 7, 2022
The fix in project-chip#12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. Ths means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the behavior in the OperationalDeviceProxy which that was defering release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discared immediately.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 7, 2022
The fix in project-chip#12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. Ths means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the behavior in the OperationalDeviceProxy which that was defering release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discared immediately.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 7, 2022
The fix in project-chip#12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. This means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the behavior in the OperationalDeviceProxy which that was defering release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discared immediately.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 7, 2022
The fix in project-chip#12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. This means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the code in the OperationalDeviceProxy that defers release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discarded immediately.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 7, 2022
The fix in project-chip#12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. This means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the code in the OperationalDeviceProxy that defers release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discarded immediately.
msandstedt
added a commit
that referenced
this pull request
Jan 7, 2022
The fix in #12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. This means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the code in the OperationalDeviceProxy that defers release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discarded immediately.
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 7, 2022
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 8, 2022
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 27, 2022
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Feb 3, 2022
step0035
pushed a commit
to hank820/connectedhomeip
that referenced
this pull request
Feb 8, 2022
The fix in project-chip#12794 means that the CASESession and PASESession objects do not need to persist for ExchangeMessageDispatch::SendMessage to succeed at the final ACK of session establishment. Instead, SendMessage uses the SessionEstablishmentExchangeDispatch global singleton. This means we can address 13146 such that CASESession and PASESession may actually be freed or reused when completion callbacks fire. This will only work, however, if these objects clear themselves as delegates for their exchange contexts when discarding references to these. This commit does so. This commit also reorders all calls to mDelegate->OnSessionEstablished and mDelegate->OnSessionEstablishmentError to occur last in any given method in case mDelegate frees or reuses the CASESession or PASESession objects on execution of these completion callbacks. With this, we can remove the code in the OperationalDeviceProxy that defers release of the CASESession object until after an iteration of the event loop. Now when OnSessionEstablished fires, the CASESession can be reused or discarded immediately.
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
ExchangeMessageDispatchinExchangeContextis dangling if CASE/PASE session is release before exchange contextChange overview
the message dispatch for case/pase is allocated inside case/pase session, but sometime the ExchangeContext holding a pointer to the dispatch can live longer than the case/pase session, due to some refer is holding by the retrans table. Causing another dangling pointer. Where triggers another crash when disposing the ExchangeContext
ExchangeMessageDispatchan empty virtual object.ApplicationExchangeDispatchandSessionEstablishmentExchangeDispatchare global singleton now, and it doesn't contain any data field.ExchangeMessageDispatchis no longer reference counted.mDispatchinsideExchangeContextbecomes a reference, which always hold a valid dispatcher.PrepareMessageandSendPreparedMessage, the implementation is same in all derived classesOnMessageReceivednon-virtual, the implementation is same in all derived classesTesting
Verified using unit-tests