fix borrowable_ptr by daschuer · Pull Request #16005 · mixxxdj/mixxx

daschuer · 2026-02-18T16:33:47Z

This is a copy of #14914 for review purpose.

Copilot

Pull request overview

This PR aims to fix a thread-safety/race issue in borrowable_ptr by changing how borrowed_ptr instances are created (via an immutable std::weak_ptr), and updates the unit test to exercise the reset/borrow interaction in a multi-threaded scenario.

Changes:

Construct borrowed_ptr via m_weakPtr.lock() and make borrow() const.
Change reset() behavior to avoid unconditional mutex operations when empty.
Update BorrowableTest.TwoThreads to reset the borrowable and add additional borrowing after a forced scheduling point.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
`src/util/borrowable_ptr.h`	Adjusts borrowing/reset implementation to address thread-safety concerns by using a `weak_ptr` for borrowing and changing reset semantics.
`src/test/borrowabletest.cpp`	Updates the concurrency test scenario to validate reset behavior and guard against deadlocks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-18T18:59:43Z

    ~borrowable_ptr() {
        reset();
-        m_mutex.unlock();
-    }
-
-    // @brief Assign a new raw pointer to the `borrowable_ptr` but not owning
-    // @param p Raw pointer to the new object.
-    // @return Reference to this instance.
-    borrowable_ptr& operator=(Tp* p) {
-        reset();
-        m_mutex.unlock();
-        m_sharedPtr = std::shared_ptr<Tp>(p, borrowable_deleter(&m_mutex));
-        return *this;
+        // m_mutex.unlock();
    }


The destructor leaves a commented-out m_mutex.unlock() behind. Please remove this dead code (and, if unlocking is no longer needed with the updated reset(), consider documenting that in a short comment instead).

what do you want to do with this?

I have removed the dead code as suggested.

Copilot · 2026-02-18T18:59:43Z

+    std::shared_ptr<Tp> m_sharedPtr; ///< Non-owning private shared pointer to the managed object.
+    // must not be changed after construction, because changing shared pointers is not thread safe.
+    const std::weak_ptr<Tp> m_weakPtr; ///< Non-owning shared pointer for sharing,


The member comment for m_weakPtr is misleading/incomplete: it’s a std::weak_ptr (not a shared pointer), and the trailing comma makes the sentence incomplete. Please update the comment to clearly describe why this weak_ptr exists and what thread-safety guarantee it provides.

Suggested change

std::shared_ptr<Tp> m_sharedPtr; ///< Non-owning private shared pointer to the managed object.

// must not be changed after construction, because changing shared pointers is not thread safe.

const std::weak_ptr<Tp> m_weakPtr; ///< Non-owning shared pointer for sharing,

std::shared_ptr<Tp> m_sharedPtr; ///< Private shared pointer to the managed object, reset only by `borrowable_ptr`.

// Must not be changed after construction, because changing shared pointers is not thread safe.

const std::weak_ptr<Tp> m_weakPtr; ///< Const weak reference to `m_sharedPtr` used by `borrow()` so that threads can

///< safely obtain temporary strong references via `lock()` without mutating `m_sharedPtr`.

daschuer

Done

ywwg

this is looking good, as far as I can understand how this works :).

ywwg · 2026-02-24T17:21:06Z

    ~borrowable_ptr() {
        reset();
-        m_mutex.unlock();
-    }
-
-    // @brief Assign a new raw pointer to the `borrowable_ptr` but not owning
-    // @param p Raw pointer to the new object.
-    // @return Reference to this instance.
-    borrowable_ptr& operator=(Tp* p) {
-        reset();
-        m_mutex.unlock();
-        m_sharedPtr = std::shared_ptr<Tp>(p, borrowable_deleter(&m_mutex));
-        return *this;
+        // m_mutex.unlock();
    }


what do you want to do with this?

to avoid a race condition with reset(). Also remove copy constructor, because writing the shared_ptr itself is not thread safe.

ywwg · 2026-03-01T20:25:40Z

-            int* p2 = borrowed1.get();
-            qDebug() << "future2" << (p1 ? *p1 : 0) << (p2 ? *p2 : 0);
+            int* p2 = borrowed2.get();
+            qDebug() << "future2 a" << (p1 ? *p1 : 0) << (p2 ? *p2 : 0);


stray debug messages?

daschuer · 2026-04-03T22:34:19Z

Ups, already merged via ma private repository

github-actions Bot added the code quality label Feb 18, 2026

ywwg requested a review from Copilot February 18, 2026 18:54

Copilot started reviewing on behalf of ywwg February 18, 2026 18:55 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

daschuer commented Feb 23, 2026

View reviewed changes

Comment thread src/test/borrowabletest.cpp

Comment thread src/util/borrowable_ptr.h

Comment thread src/util/borrowable_ptr.h

daschuer mentioned this pull request Feb 23, 2026

Fix borrowable_ptr #14914

Merged

ywwg approved these changes Feb 24, 2026

View reviewed changes

daschuer force-pushed the borrowable_ptr branch from d78d648 to d3fba27 Compare February 24, 2026 21:19

daschuer added 5 commits February 28, 2026 23:15

Use an immutable std::weak_ptr for creating borrowed_ptr

599ad6e

to avoid a race condition with reset(). Also remove copy constructor, because writing the shared_ptr itself is not thread safe.

Add an explaining comment in BorrowableTest

38a775f

Fix typo in BorrowableTest debug output

99c3773

Disable copy and move explicite in borrowable_ptr

22247cd

Improve comments.

91f2a0f

daschuer force-pushed the borrowable_ptr branch from d3fba27 to 91f2a0f Compare February 28, 2026 22:15

ywwg reviewed Mar 1, 2026

View reviewed changes

Disable debug

b8c982d

daschuer closed this Apr 3, 2026

daschuer deleted the borrowable_ptr branch April 3, 2026 23:51

-    std::shared_ptr<Tp> m_sharedPtr; ///< Non-owning private shared pointer to the managed object.
-    // must not be changed after construction, because changing shared pointers is not thread safe.
-    const std::weak_ptr<Tp> m_weakPtr; ///< Non-owning shared pointer for sharing,
+    std::shared_ptr<Tp> m_sharedPtr; ///< Private shared pointer to the managed object, reset only by `borrowable_ptr`.
+    // Must not be changed after construction, because changing shared pointers is not thread safe.
+    const std::weak_ptr<Tp> m_weakPtr; ///< Const weak reference to `m_sharedPtr` used by `borrow()` so that threads can
+                                       ///< safely obtain temporary strong references via `lock()` without mutating `m_sharedPtr`.

Uh oh!

Uh oh!

Conversation

daschuer commented Feb 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

ywwg Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

daschuer Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ywwg left a comment

Choose a reason for hiding this comment

Uh oh!

ywwg Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

ywwg Mar 1, 2026

Choose a reason for hiding this comment

Uh oh!

daschuer Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

daschuer commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants