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

Modify toSpanData to happen inside attributesLock to prevent data races #632

Conversation

ArielDemarco
Copy link
Contributor

Overview

This PR addresses a potential data race issue when toSpanData() is called concurrently with modifications to attributes by wrapping both operations within the same lock.

Explanation

When somebody needs to call toSpanData during the lifetime of a Span, the toSpanData() method accesses attributes and totalAttributeCount while another thread may be modifying them (each time an attribute is added/removed totalAttributeCount is also modified), leading to potential crashes or memory corruption.

Fixes #630

@nachoBonafonte
Copy link
Member

Sorry I just realized that it must be locked also with the other lock (the event one) as also events could be modified in while calling it ( this pattern is also used in the stop method)

@ArielDemarco
Copy link
Contributor Author

Sorry I just realized that it must be locked also with the other lock (the event one) as also events could be modified in while calling it ( this pattern is also used in the stop method)

You're right! that lock also wraps properties that are going to be used by the toSpanData method so it should be wrapped by that lock too. Already made the commit

@ArielDemarco
Copy link
Contributor Author

Seems like adding the eventsSyncLock causes a deadlock (that's why tests are failing), cause inside of the withLock in toSpanData there's a call to adaptEvents() which also uses the eventsSyncLock. Seems that every other thing locked with the eventsSyncLock is also being called directly from the toSpanData (adaptEvents, getTotalRecordedEvents).
So maybe is safe to say that the best thing to do is to remove the eventsSyncLock usage in this method? what do you think @nachoBonafonte ?

@nachoBonafonte
Copy link
Member

Yes, if all the calls being made by the method are covered then it is not needed. It was my fault asking you to add it, I should have checked first, sorry.

@ArielDemarco
Copy link
Contributor Author

No problem! did the rollback! Hope next run works fine

@nachoBonafonte nachoBonafonte merged commit 0cd4a4c into open-telemetry:main Nov 7, 2024
6 checks passed
@nachoBonafonte
Copy link
Member

It worked with last changes so I merged. Thanks a lot.

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.

Concurrency issues with SpanData modification leading to vrashes
2 participants