borrowable_ptr, a threadsave callback solution

[daschuer]

A wrapper class to protect pointers owned by another entity, allowing
safe access to the referenced object across threads. This prevents the
object from being deleted by its owner while other threads are executing
functions on the referenced object, implemented mostly lock-free, except during
reset(), if the object is still in use.

Use cases are:

• Implementing callbacks to objects with shorter lifetimes.
• A safe replacement for Qt's direct connections, which must not be used across
  threads in such cases.

Unlike std::shared_ptr, the object is not deleted when the last
borrowed_ptr falls out of scope. Ownership and deletion are always managed
by the original owner (via borrowable_ptr).

If the borrowable_ptr falls out of scope while there are active borrowed_ptr
instances, the thread is suspended until all borrowed_ptr instances are deleted
and the reference count reaches zero.

Thread safety:

• It is thread-safe to replace the object in borrowable_ptr.
• The transition is not atomic: borrowed_ptr instances become null before pointing
  to the new object.

Usage:

	int i = 5;
	{
	    // Make i borrowable, owned by the stack
            auto borrowable = borrowable_ptr(&i);
	    {
	        // Borrow i, this can be borrowd across threads
                // borrowed_ptr is a strong reference that guranties the borrowed object is
                // valid unit the end of the scope
                borrowed_ptr borrowed1 = borrowable.borrow();
	        borrowed_ptr borrowed2 = borrowable.borrow();
	    } // borrowed objects are returned
	} // borrowable_ptr falls out of scope, suspended if a borrowed pointer is not returned

[Be-ing]

Could you explain what specific problem motivated you to start working on this? Does it solve a problem that came up in #1675?

[daschuer]

I am experience rar crashes when shutting down Mixxx.I was never able to reproduce it under gdb, so I have never reported a bug. One idea is that this is caused by explicit direct connections across threads. Qt does not prevent to call a callback from an already deleted object, if there is a race between the callback call from one thread and the delete call from an other thread.
If we use this new use_counting_ptr, the deleting thread is locked until the callback has returned.

[daschuer]

We use these direct connection for the engine thread COs

[daschuer]

I cannot use this currently, because of pending #1700

[Be-ing]

Can we come back to this in a few weeks after the 2.2 feature freeze?

[daschuer]

I like to have this included.

[Be-ing]

Why?

[daschuer]

I actually waiting for a review of #1700 more urgently. Our control objects where becoming a bit messy, and I think it is a good idea to clean them up in small steps instead of one giant PR.
This one so should hopefully solve the pending shut down crashes, finally.

[Be-ing]

I'm not questioning that we should clean the code, but I don't think < 2 weeks until a feature freeze is a good time to be focusing on it unless it fixes an important bug.

[daschuer]

@rryan Do you want to have a look here?
I like to have this new pointer to wrap the m_pCreatorCO here:
https://github.com/mixxxdj/mixxx/blob/master/src/control/control.h#L164

[daschuer]

@uklotzde do you have interest to wrap your head around this? I would like it to fix the a probably cause of the crash on close fix I experience some times.

[uklotzde]

Honestly I still don't get the point by reading the code??

Couldn't this be implemented in a simpler and more comprehensible way? I'm not able to decide if this works correctly in all possible cases. My feeling is that we might try to cover a design flaw by mitigating it with just another half-baked and fairly complex solution.

I noticed that the implementation has some technical flaws, e.g. the constructors are inconsistent and behave unexpectedly when invoked with a nullptr.

I came to the conclusion that we should use asynchronous signal/slot messaging whenever possible. Why plain callbacks? Trying to work around Qt's foundation and patterns is one of the main problems in our code base. I'm just criticizing myself, because this is what I would've done when rewriting the multi-threaded analysis code now. But I won't rewrite it a second time in C++ ;)

The global track cache is a slightly different story. It's also far too complex, but since track objects appear as shared mutable state everywhere we didn't have a choice without rewriting large parts of the application.

[daschuer]

Thank you for looking into it.

This pointer was my idea for solving a crash on shut down when this https://github.com/mixxxdj/mixxx/blob/master/src/control/control.h#L164 object is deleted while an other thread is just about to call callback.

I would love to use a Qt solution for it, but it has not. Direct connections have no safety net in QT and suffer basically the same issue.

My idea was to use this pointer type to workaround this limitation, in an almost look free way.
The idea was to use this new pointer, for a maybe unusual but working solution of minimal scope.

Do you have alternative ideas? Is there a chance to tun this into a simpler and more comprehensible solution? That would be great.

[uklotzde]

Would you please include an example were this technique is applied? I need to see it in action. We need this anyway. Adding unused and untested code to the repository is discouraged ;)

[daschuer]

I have started to use the new pointer here: https://github.com/mixxxdj/mixxx/blob/master/src/control/control.h#L164
But it is becoming a great mess, because it is copied and stored all over the Mixxx code base.
We need to fix that first.

Many instances can be replaced by ControlProxyLt #1717

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.

[daschuer]

I have implemented the original solution on top of std::shared_ptr

please have a look.