atilaneves / automem

C++-style automatic memory management smart pointers for D
BSD 3-Clause "New" or "Revised" License
86 stars 15 forks source link

RefCounted release is not atomic #63

Open submada opened 3 years ago

submada commented 3 years ago

RefCounted.release method is not atomic.

    void release() {
        /*
        ...
        */

        dec;
        if(_impl._count == 0) {
            /*
            ...
            */
        }
    }

calling "dec" is atomic and "_impl._count == 0" is atomic but together they are not atomic.

Something like this is necessary:

if(atomicFetchSub!(MemoryOrder.acq_rel)(_impl._count , 1) == 1){ //... } Clang use it too: https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L3398

submada commented 3 years ago

This code show that in multithread enviroment RefCounted can call destructor and dealloc multiple times for same object.

import core.atomic : atomicFetchAdd, atomicFetchSub, atomicLoad, atomicStore;
import std.experimental.allocator.mallocator : Mallocator;
import std.concurrency : spawn;
import std.meta : AliasSeq;
import std.stdio : writeln;
import std.conv : to;

import automem.ref_counted;

__gshared int counter = 0;
__gshared ulong iteration = 0;

alias Rc(T) = RefCounted!(shared(T));

struct Foo{
    int i;
    this(int i)shared {
        this.i = i;
        atomicFetchAdd(counter, 1);
    }

    ~this(){
        atomicFetchSub(counter, 1);
    }
}

void foo(shared(Rc!Foo)* ptr, const ulong iteration, shared(uint)* ready){
    {
        Rc!Foo x = *cast(Rc!Foo*)ptr;                /// copy ref counted object

        atomicFetchAdd(*ready, 1);                   ///inform main thread that this thread is ready to new iteration

        while(atomicLoad(.iteration) != iteration){} ///wait until main thread start iteration

        ///destruction of ref counted object
    }
    atomicFetchSub(*ready, 1);      ///inform main thread that this thread ended iteration
}

void main(){

    foreach(ulong i; 1 .. 30){
        shared x = Rc!Foo(-1);
        shared uint ready = 0;

        spawn(&foo, &x, i, &ready);
        spawn(&foo, &x, i, &ready);

        while(atomicLoad(ready) != 2){}     ///wait until both threads make copy of x.

        (*cast(Rc!Foo*)&x) = Rc!Foo.init;   ///release x

        atomicStore(iteration, i);          ///start new iteration

        while(atomicLoad(ready) != 0){}     ///wait until both threads end iteration.
    }

    assert(counter >= 0, "double destruction: " ~ (-counter).to!string);
    assert(counter == 0, "missing destruction" ~ counter.to!string);
}