[api/logs] Return NoopLogRecord from NoopLogger #2662
Closed
Conversation
This file contains hidden or 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
Signed-off-by: Yuri Shkuro <github@ysh.us>
marcalff
reviewed
May 7, 2024
Co-authored-by: Marc Alff <marc.alff@free.fr>
marcalff
reviewed
May 7, 2024
3 tasks
marcalff
requested changes
May 14, 2024
There was a problem hiding this comment.
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 <github@ysh.us>
|
@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 <github@ysh.us>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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.mdupdated for non-trivial changes