Not call onError in BufferedReadCallback #30965
Conversation
src/app/BufferedReadCallback.cpp
Outdated
| mCallback.OnError(err); | ||
| ChipLogError(DataManagment, "Fail to dispatch buffered data with error %s in path %s", err.toString(), mBufferedPath.LogPath()) |
There was a problem hiding this comment.
Why is this the right thing? Under what conditions does DispatchBufferedData fail, and in those cases should we in fact press on, or should we in fact error out?
There was a problem hiding this comment.
Regarding the error condition,
Looking at the DispatchBufferedData, further GenerateListTLV, it could fail to calloc with buffer and other error inside GenerateListTLV.
If OnReportBegin and OnReportEnd fails, we need Error out in readClient since these error could be fatal one, not worth moving on, maybe we should change API shape for onReportBegin and onReportEnd from
void OnReportBegin() override;
void OnReportEnd() override;
to
CHIP_ERROR OnReportBegin() override;
CHIP_ERROR OnReportEnd() override;
There was a problem hiding this comment.
Is there a way to simulate such cases in a unit test?
It seems a bit uncomfortable to create a change deep inside our core stack without a corresponding test that shows "this was bad before, it is good now".
There was a problem hiding this comment.
@yunhanw-google We could set a flag in the buffered read callback that returns error for the next call from ReadClient, if we just want to treat these as completely fatal errors....
That is, keep doing OnError, but after that respond to all calls from the readclient with errors and not notify our consumer anymore.
fixed typo
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
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. onError should be called only with close path from readClient.