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.

Answer 1

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();
}