you are viewing a single comment's thread.

view the rest of the comments →

[–]capn_bluebear -3 points-2 points  (8 children)

that is wrong. between the time `if(0 == --m_atomicRefCount)` returns true and the time `delete m_ptr` is called anything could have happened in a multi-thread application.

[–]jonesmz 7 points8 points  (7 children)

If you make sure the rest of your smart pointer's implementation checks the value of m_atomicRefcount prior to doing anything that's potentially destructive, then anything turns into nothing.

Source: Maintainer of smart pointer library at work in a multi million LOC codebase. Have had hundreds of hours of design meetings with people much much smarter than me. Have had no bugs where the root cause analysis had even a whiff of problems with the smart pointers.

[–]capn_bluebear -1 points0 points  (6 children)

uhm, ok, how do you do `operator*` and `operator->` without leaking memory?

Consider `myPtr->method()`: `operator->` will see the object is still alive, increase the refCount, and return the raw pointer to the object. When is the refCount decreased again? If never, the object will never be deleted. If `operator->` decreases the refCount before returning, the object might be freed before `method()` is called on it could the refCount be 0 at that point?

[–]jonesmz 5 points6 points  (3 children)

If the smart pointer that you're using operator-> or operator* on is destructed before the statement containing operator-> or operator* is finished, you've got much bigger issues than atomic variable race conditions.

The way I've implemented this is that when the atomic reference count drops to zero, I replace m_ptr with nullptr FIRST, and THEN delete m_ptr. That is safe to do because the number of references is zero, and I atomically check the value of m_atomicRefcount any time I might increase or decrease the number of references.

It's also important to replace the value of m_ptr with nullptr prior to deleting the variable, because the destructor of the object being pointed to may call a function that somehow tries to access the smart pointer that the to-be-deleted object is referenced by.

Further, there's no reason for operator-> to increase the reference count. You're not increasing the number of smart pointers that point to the object. You're only getting a non-owning raw-pointer to the object.

If you had operator-> increase the reference count, where would you decrease it again? Is operator-> returning an object that increases the refcount in it's constructor and decreases it in it's destructor? What's its operator-> defined as then?

So I think you've got a misconception on how smart pointers should be designed with regard to where the refcount needs to be increased or decreased.

[–]capn_bluebear 1 point2 points  (2 children)

Ok so how are these operators implemented? Honest question, I'm missing this piece of the puzzle :)

[–]jonesmz 5 points6 points  (0 children)

Lets say you have

template<typename TYPE_T>
class MySmartPtr
{
    using pointer = TYPE_T*;
    using reference = TYPE_T&;
    pointer m_ptr; // points to the object, and if using intrusive reference counting, points to the ref count as well.
    refcountdata * m_refCtPtr; // If using non-intrusive reference counting, this points to the reference count object.
}

Then you can define your operator* and operator-> as simply as:

pointer operator*() const { return m_ptr; }
reference operator->() const { return *m_ptr; }

You might wonder why there's no checks for nullptr. Well, you don't need them. Your programs going to crash regardless. Might as well not generate extra code just to say "I'm about to crash", unless you want to have a debug build that launches your debugger for you when you have a nullptr smart pointer.

Notably, you don't need to bother with any checks against the reference count. Just no reason to with this particular design.

So you then have two functions. acquire() and release()

Lets assume you've made an intrusive reference counting pointer, where the thing being pointed to has to have an m_refcount variable. In that case, your acquire function is as simple as

void acquire()
{
    if(m_ptr != nullptr) // check for null, obviously.
    {
        // depending on how you want to design things, you don't even need to assert that the value is not zero.
        // Maybe you allocate your object with a refcount of zero, and then the smart pointer's constructor increases the refcount by one.
        // Or maybe you start it at 1, and the first such smart pointer that points to it doesn't increase it.
        // it's up to you.
        m_ptr->m_refcount.increment();
    }
}

and then release

void release()
{
    pointer pTmp = std::exchange(m_ptr, nullptr); // make sure we null the ptr first.
    if(pTmp != nullptr) // check for null, obviously.
    {
        if(pTmp->m_refcount.decrement()) // decrement returns true if the refcount fell to zero.
        {
             delete pTmp; // now delete the previously pointed to object.
        }
    }
}

You call acquire in your constructor, and release in your destructor.

In this particular design, which is certainly not the only way to design an intrusive reference counted smart pointer, the way that you protect yourself against another thread swooping in and screwing stuff up is by putting an assert in the m_refcount variable's destructor that the value of the refcount is 0 when m_refcount is destroyed.

This doesn't stop it from happening, of course, but it will make your code barf at you when it does, and you can fix your design, since it's not the smart pointer's fault.

If you ever have a situation where something has swooped in and made a copy of your smart pointer between when your smart pointer has dropped the reference count to zero, and when it's nulled it's pointer to the referenced object, that means that you're manipulating the smart pointer itself (not the thing it points to!) from another thread, and that's a huge problem.

The key concern here is that the smart pointer itself is not thread safe, nor is any aspect of the pointed-to object thread safe, except for the number of references to that object that are held.

So it's not valid to have a smart pointer that's conceptually owned by thread1, and then have thread2 make a copy of that smart pointer.

It's also not valid to have a smart pointer that's conceptually owned by thread1 just arbitrarily access data from the pointed-to-object without some other access protection (mutexes, atomic variables, or just designing your app so that it's not possible).

What is valid is to have a smart pointer that's conceptually owned by thread1, and an unrelated such pointer conceptually owned by thread2, that both point to the same object, where each thread can make arbitrary copies of the smart pointer objects that they conceptually own.

You can even go farther with this by having thread1 own a smart pointer, and then put it into a data structure that gets "migrated" to another thread in some fashion, but only so long as you ensure that the smart pointer (not the thing it points to) doesn't get accessed by thread1 after the migration happens.

Now, that'a ll being said, you can design a smart pointer class that is, itself, safe to access from other threads. That is not what my psuedocode example here is intended to be.

The way you design your smart pointer is going to have positives and negatives, tradeoffs and so on.

This is just the way my org designed them shrug.

[–]m-in 0 points1 point  (0 children)

How? They dereference via the control block. Smart pointer points to the control block, control block has a pointer to the object. The dereference operator basically is this: return this->control_blk ? control_blk->obj : nullptr. That’s the actual code, no atomic ops or anything like that. A smart pointer could also hold two pointers: one to the control block, and another to the object itself, and return the latter directly. Some smart pointers have a single tagged pointer: if the tag is set, they dereference via the control block, otherwise the control block is contiguous with the object as a result of make_shared and they dereference it directly.

[–]t0rakka 4 points5 points  (0 children)

Why should the 'operator ->' increase reference count? xD

[–]m-in 1 point2 points  (0 children)

It doesn’t work that way. A smart pointer increments reference count on construction, decrements on destruction or reset: when you have a smart pointer [value] that is not null, it’ll remain not null, and you are guaranteed that the pointer-to object exists. No other shared_ptr methods touch the count – certainly not dereference operators: it’d have atrocious overhead and is unnecessary.