-
Notifications
You must be signed in to change notification settings - Fork 775
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
[21672] Fix data race in TypeObjectFactory::get_instance
#5238
Conversation
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
771f9a3
to
81eb7a0
Compare
3e59261
to
b0c9112
Compare
b0c9112
to
1fd1489
Compare
1fd1489
to
95c6e66
Compare
95c6e66
to
1bb99ab
Compare
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
…test fail. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
This way we have a single final expectation. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
…iltin_annotations_types`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
1bb99ab
to
9039ad7
Compare
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.
Good catch, leaving a NIT and a suggestion
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
…uctor. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
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
@MiguelCompany could you confirm that the failing test are flaky in 2.10.x
?
The only one that seems new is |
@Mergifyio backport 2.14.x |
✅ Backports have been created
|
* Refs #21664. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Improve synchronization in regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Return created instance to visualize data-race and make test fail. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Just count the number of different instances. This way we have a single final expectation. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Avoid using g_instance inside the instance. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Inject factory in all methods called inside `register_builtin_annotations_types`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Use atomic enumeration to control the instance state. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Uncrustify. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. Notify condition from main thread. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. FIx EOL at end of file. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. Refactor to create builtin objects inside factory constructor. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. Uncrustify. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 8f4b4a5) # Conflicts: # src/cpp/dynamic-types/TypeObjectFactory.cpp
* Fix data race in `TypeObjectFactory::get_instance` (#5238) * Refs #21664. Regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Improve synchronization in regression test. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Return created instance to visualize data-race and make test fail. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Just count the number of different instances. This way we have a single final expectation. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Avoid using g_instance inside the instance. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Inject factory in all methods called inside `register_builtin_annotations_types`. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Use atomic enumeration to control the instance state. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21664. Uncrustify. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. Notify condition from main thread. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. FIx EOL at end of file. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. Refactor to create builtin objects inside factory constructor. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> * Refs #21672. Uncrustify. Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> (cherry picked from commit 8f4b4a5) # Conflicts: # src/cpp/dynamic-types/TypeObjectFactory.cpp * Fix conflicts Signed-off-by: Miguel Company <miguelcompany@eprosima.com> --------- Signed-off-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Description
While debugging an error reported by LeakSanitizer in #4916, an important data race was discovered in
TypeObjectFactory::get_instance
.The test that PR adds is creating several writers from different threads simultaneously.
This leads to calling
TypeObjectFactory::get_instance
when registering the local writer (during the creation of itsWriterProxyData
).The data race made several factories to be constructed, but only one was assigned to the global g_instance pointer.
Only one of the factories was destroyed when the process finished, making LeakSanitizer complain.
This PR adds a mechanism based on an atomic enumeration holding the state of the singleton instance.
@Mergifyio backport 2.14.x
Contributor Checklist
versions.md
file (if applicable).Reviewer Checklist