Referencing this pointer in constructor

Question

Following is another example of forward declaration, which might be useful if the application needs a self-sustaining array of objects which is able to add and remove objects from itself during run-time:

File a.h:
class A {
public:
    static A *first, *last;
    A *previous, *next;

    A();
    ~A();
};
File a.cpp:
#include "a.h"

A *A::first=0, *A::last=0; // don't put the word static here, that will cause an error

A::A() {
    if(first==0) first=this; //first A created
    previous=last;
    if(previous != 0) previous->next=this;
    last=this;
    next=0;
}

A::~A() {
    if(previous != 0) previous->next=next;
    if(next != 0) next->previous=previous;
}

From : https://en.wikipedia.org/wiki/Circular_dependency#Self-reference_example

I think implementation of A, has an issue. While first instance of A is being constructed, if some other thread references A::first, it will lead to unexpected behaviour. Please correct if I am wrong.

Also, how can this issue be resolved? Thanks.

Yes this is indeed a problem, the code is not thread-safe. To fix this you need some kind of locking, like a semaphore. — Some programmer dude, Dec 14 '15 at 12:51
It is definitely not thread-safe. Another problem (not related to threads) is that constructor sets `first` and `last`, but destructor does not modify them. I think there should be some symmetry there. — zvone, Dec 14 '15 at 12:56
The problems of this example are numerous, such as non-existent encapsulation, lack of ctor/dtor symmetry, lack of thread safety, and lack of flexibility. — Sebastian Redl, Dec 14 '15 at 15:40

score 2 · Accepted Answer · answered Dec 14 '15 at 14:46

This code is definitely non thread-safe as others stated in comments. Be it first object or not, it 2 threads try to create or delete a A object at the same time, you get undefined behaviour, because two different threads use and change same static value without any synchronization.

What could be done? As always to two same options:

document the class (at least contructor and destructor) as not being thread safe. Since them it is the caller's responsability to ensure that only one thread can access A objects at the same time. Say differently, it means that A will be safe only in a single treaded program.
add synchronization inside the class itself to make it thread safe. As creation and destruction manipulate static members, you need a global mutex for all objects of the class, say differently a class static one.

And as @zvone noticed in comment, the destructor could make first and last become dangling pointers when you delete first or last element of the chain.

Destructor (non thread safe version) should be:

A::~A() {
    if(previous != 0) previous->next=next;
    if(next != 0) next->previous=previous;
    if (first == this) first = next;
    if (last == this) last = previous;
}

A thread-safe version could be:

File a.h

class A {
public:
    static A *first, *last;
    A *previous, *next;
    static std::mutex mut;

    A();
    ~A();
};

File a.cpp:

#include "a.h"

A *A::first=0, *A::last=0; // don't put the word static here, that will cause an error
std::mutex A::mut;

A::A() {
    mut.lock()
    if(first==0) first=this; //first A created
    previous=last;
    if(previous != 0) previous->next=this;
    last=this;
    next=0;
    mut.unlock();
}

A::~A() {
    mut.lock()
    if(previous != 0) previous->next=next;
    if(next != 0) next->previous=previous;
    if (first == this) first = next;
    if (last == this) last = previous;
    mut.unlock();
}

score 1 · Answer 2 · answered Dec 14 '15 at 14:55

The problem with the answers here is that they don't point to the fact that previous and next are public members - which means, no protection by mutextes within constructor and destructor can make the class fully thread safe. The only way to make this class thread-safe is to hide those members and provide methods to access to them - each having a synchronization primitive inside.

score 0 · Answer 3 · answered Dec 14 '15 at 12:55

0

You need to declare first and last variables as atomic using for example std::atomic
Or protect them with mutex

answered Dec 14 '15 at 12:55

Roman Zaitsev

1,328
5
20
28

2

Declaring variables as atomic doesn't automatically make your code thread-safe. For example, `if(first==0) first=this;` consists of two operations so atomicity of `first` won't help. – interjay Dec 14 '15 at 14:58
@interjay: True, but one could do `atomic_compare_exchange(&first, nullptr, this)` (well, you'll need the address of a local variable holding `nullptr` for the second parameter, but the approach is valid) – Ben Voigt Dec 14 '15 at 15:00
@BenVoigt Yes, that would work for that specific line, but the rest becomes much harder. Correctly implementing a lockless doubly-linked list is possible but quite difficult, and not as trivial as this answer makes it seem. – interjay Dec 14 '15 at 15:05
@interjay You are right, lock-free structures are hard to code ( and wait-free are much harder ), but I mentioned it to show all possibilities. – Roman Zaitsev Dec 15 '15 at 06:31

score 0 · Answer 4 · answered Dec 14 '15 at 12:57

Yes that's right. They Need to be protedted. Additionally, previous and next Need to be protected too. e.g.

A::~A() {
    unique_lock<std::mutex> locked(A::A_mutex);
    if(previous != 0) previous->next=next;
    if(next != 0) next->previous=previous;
}

where A_mutex is a static member.

Referencing this pointer in constructor

4 Answers4