Skip to content

Add test to illustrate thread safety issue if used wrongly#9

Open
hodlinator wants to merge 1 commit intostlab:mainfrom
hodlinator:2026/01/thread_issue
Open

Add test to illustrate thread safety issue if used wrongly#9
hodlinator wants to merge 1 commit intostlab:mainfrom
hodlinator:2026/01/thread_issue

Conversation

@hodlinator
Copy link

@hodlinator hodlinator commented Feb 2, 2026

I spotted a potential footgun while reviewing an integration of copy_on_write into a project which does not have a history of enforcing value-semantics (bitcoin/bitcoin#34424). The footgun shouldn't be firing that often, but I still thought it would be worth documenting with a test.


The initial description in copy_on_write.hpp states:

a thread-safe copy-on-write wrapper for any type that models Regular

If Regular implies value-semantics in a way that makes this issue impossible, it might be useful to replicate a description of those constraints in this repository. Or link to a preferred definition.


The issue could be mitigated somewhat by changing the write()-overloads to be more restrictive (return void and assert(unique()) after mutation):

    template <class Inplace>
    void write(Inplace inplace) {
        static_assert(std::is_invocable_r_v<void, Inplace, T&>,
                      "Inplace must be invocable with T&");
        if (!unique()) *this = copy_on_write(read());
        inplace(_self->_value);
        assert(unique()); // Catch invalid multi-threading usage.
    }

    template <class Transform, class Inplace>
    void write(Transform transform, Inplace inplace) {
        static_assert(std::is_invocable_r_v<T, Transform, const T&>,
                      "Transform must be invocable with const T&");
        static_assert(std::is_invocable_r_v<void, Inplace, T&>,
                      "Inplace must be invocable with T&");

        if (!unique()) {
            *this = copy_on_write(transform(read()));
        } else {
            inplace(_self->_value);
        }
        assert(unique()); // Catch invalid multi-threading usage.
    }

But I understand that would be prohibitive for existing projects.

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.

1 participant