-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[IM] Handle internal fatal error in BufferReadCallback #36187
base: master
Are you sure you want to change the base?
[IM] Handle internal fatal error in BufferReadCallback #36187
Conversation
f615c29
to
a75bf72
Compare
PR #36187: Size comparison from 9ee0499 to 2167712 Full report (67 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bb45a1c
to
7a20bd8
Compare
PR #36187: Size comparison from fef41bd to 7a20bd8 Full report (3 builds for cc32xx, stm32)
|
7a20bd8
to
44484ae
Compare
PR #36187: Size comparison from fef41bd to 44484ae Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
src/app/ReadClient.h
Outdated
/* | ||
* Get the last internal fatal error in callback | ||
*/ | ||
virtual CHIP_ERROR GetLastError() const { return CHIP_NO_ERROR; } |
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.
Why is this new API needed? This is making all ReadClient consumers pay extra flash costs for an internal implementation details of BufferedReadCallback (which is meant for "capable clients" only, and is better able to shoulder such costs).
If this is trying to implement the suggestion from #30965 (comment), then that's not what the suggestion was. The suggestion was that BufferedReadCallback store some internal state that will allow it to maintain the relevant invariants and just start ignoring calls from ReadClient after it reaches that error state, no? As far as the ReadClient is concerned, there would be no changes in behavior of any sort here.
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 we introduce GetLastError as new callback in ReadClient, we check GetLastError in the end of onAttributeData or onEventData in ReadClient, no matter what kind of chained callback we have, we could lead to Close call in readClient for read or subscribe, for resubscription case, it would depend on which error OnResubscriptionNeeded returns, OnResubscriptionNeeded is not only conveying this error to contrloller applicaiton, but also decides how to translate the error, for example, application can skip all errors, or application can treat the errors except timeout as no error, in all situations, onError would only be called inside ReadClient, only inside Close, we may also update comments for onError, and clearly mention onError is not expected called outside ReadClient.
1003774
to
b5e7840
Compare
src/app/BufferedReadCallback.cpp
Outdated
} | ||
|
||
mLastError = DispatchBufferedData(mBufferedPath, StatusIB(), true); | ||
ReturnOnFailure(mLastError); |
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.
OK, so this skips calling OnReportEnd and removes the OnError call. But API guarantees either OnReportEnd or OnError, no? I don't understand how this is supposed to work.
That said, in the subscription case OnError means something quite specific which does not match what this code ends up doing if we did call OnError.
So what is the actual behavior we are trying for here?
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.
sorry, I forgot to push latest change, could you re-review?
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.
Again: "what is the actual behavior we are trying for here?" For reads, and for subscriptions (which may want different behaviors, because OnError has very different meanings for them).
2ad287b
to
d61033c
Compare
PR #36187: Size comparison from d93f3f6 to a2d22ca Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bcdf800
to
8940d43
Compare
PR #36187: Size comparison from 50ad31c to fb3289e Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #36187: Size comparison from 50ad31c to 81bf7fd Increases above 0.2%:
Full report (67 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
b24d465
to
ab685c6
Compare
ab685c6
to
4f893c6
Compare
Revive #30965
Issue: If onError has been called from BufferedReadCallback when processing multiple attrbitues, potentially onError or onAttribute or onEvent could be called again from ReadClient, as a result, the behavior in application could be unexpected, crash is possible. note: the errors in BufferredReadCallback are treated as fatal ones.
Solution: In order to resolve the above issue, we add mDataBufferingError to store internal error in BufferredReadCallback, we call a new introduced GetLastError callback in readClient after each onAttribute and onEvent, if any fatal error happens, we propagate the error to Close in ReadClient, for any new callback class that inherits ReadClient::Callback, if there is a fatal error, ReadClient can get aware of this error from GetLastError.