Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented May 7, 2024

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
  • Unit tests have been added
  • Changes in public API reviewed

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Marc Alff <marc.alff@free.fr>
Copy link
Member

@marcalff marcalff left a 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 <github@ysh.us>
@yurishkuro
Copy link
Member Author

@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>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro closed this May 14, 2024
@yurishkuro yurishkuro deleted the noop-logger branch May 14, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API] NoopLogger causes segfault
2 participants