-
Notifications
You must be signed in to change notification settings - Fork 438
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
[api/logs] Return NoopLogRecord from NoopLogger #2662
Conversation
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Marc Alff <[email protected]>
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.
Thanks for the PR.
The idea to share a pointer on a dummy noop record, to avoid new + delete, unfortunately does not work, and causes the runtime failures seen in CI.
When logging 2 events in a row:
- CreateLogRecord() for event 1 returns a dummy record
- Delete log_record for event 1 calls the destructor, which invalidates the dummy record virtual table
- CreateLogRecord() for event 2 returns a damaged dummy record
- record->SetXXX() crashes when invoking virtual methods.
Implementing operator new() and operator delete() instead only moves the problem elsewhere, because the same crash will happen when 2 threads use the same dummy record concurrently.
Signed-off-by: Yuri Shkuro <[email protected]>
@marcalff then I suggest going back to allocating a new Noop record, that at least prevents the existing behavior that returns nullptr unexpectedly, and address performance issue separately. |
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Fixes #2656
Changes
NoopLogger was incorrectly returning nullptr, causing segfaults in client code. After this change it returns a dummy no-op log record.
CHANGELOG.md
updated for non-trivial changes