-
Notifications
You must be signed in to change notification settings - Fork 140
RUM-7543 Allocate rwlock on the heap #2301
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
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 51 Passed, 3847 Skipped, 2m 0.35s Total duration (2m 32.45s time saved) |
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.
Nice find! Thanks for this fix. 🛠️ I wonder how often this lead to unexpected crashes given that we use this lock in many parts of the SDK.
@mariedm I don't expect it to be much of a problem in its current state since the lock is wrapped within a class, but still it was at risk. |
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.
Can you add more tests to the ReadWriteLockTests
? For instance, tests that trigger the ReadWriteLock deinit
concurrently?
@simaoseica-dd, the |
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.
LGTM
/merge |
View all feedbacks in Devflow UI.
The expected merge time in
|
What and why?
Given this exemple:
rwlock
is not a real pointer that is long-lasting valid. Instead the lock pointer should be allocated on the heap.Review checklist
make api-surface
)