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

Review LazyObjectImpl::obj() #696

Closed
alandefreitas opened this issue Oct 10, 2024 · 1 comment · Fixed by #716
Closed

Review LazyObjectImpl::obj() #696

alandefreitas opened this issue Oct 10, 2024 · 1 comment · Fixed by #716
Assignees

Comments

@alandefreitas
Copy link
Collaborator

alandefreitas commented Oct 10, 2024

Most compilers don't implement __cpp_lib_atomic_shared_ptr, on which LazyObjectImpl depends:

ObjectImpl&
LazyObjectImpl::
obj() const
{
#ifdef __cpp_lib_atomic_shared_ptr
auto impl = sp_.load();
if(impl)
return *impl;
impl_type expected = nullptr;
if(sp_.compare_exchange_strong(
expected, construct().impl()))
return *sp_.load();
return *expected;
#else
return *sp_;
#endif
}

For this reason, the function above contains two implementations, depending on whether __cpp_lib_atomic_shared_ptr is supported.

Both implementations are problematic:

  • The implementation is not thread-safe when __cpp_lib_atomic_shared_ptr is unsupported. The object would need to carry a pointer to mutex for that.
  • The implementation when __cpp_lib_atomic_shared_ptr is supported is also unsafe because the load and exchange operations are independent. This means two concurrent threads will attempt to construct the object twice if they call the function before the object is constructed.
@alandefreitas alandefreitas moved this to Accepted in MrDocs Oct 10, 2024
@vinniefalco
Copy link
Member

sad

@alandefreitas alandefreitas self-assigned this Oct 15, 2024
alandefreitas added a commit to alandefreitas/mrdocs that referenced this issue Oct 26, 2024
@github-project-automation github-project-automation bot moved this from Accepted to Done in MrDocs Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants