FFTRepo singleton is not thread-safe

[Peter Steinbach]

Hi, I just saw this interesting singleton implementation: https://bitbucket.org/multicoreware/hcfft/src/094e4e8a4c01b3563388241aa81f05a955c54b06/lib/include/hcfftlib.h?at=master&fileviewer=file-view-default#hcfftlib.h-657

the getInstance method is per se not thread-safe. even though you have a lock "hopefully" in all FFTRepo member functions this may lead to race conditions. As you don't hand out a C API yet, I suggest moving to C++11 or higher if possible. hcfft requires ROC and thus comes with clang 3.5 or higher which has very good support for C++11. Using std::atomic and friends would result in lower maintenance overhead and yield a portable implementation.

[Neelakandan Ramachandran]

@psteinb C++11 standard guarantees thread safe initialization of static variables right ?

Seems Static Initializer is one of the many propogated approach to circumvent Double-Checked Locking Pattern

Relevant links that I have gone through for this

http://stackoverflow.com/questions/2576022/efficient-thread-safe-singleton-in-c

http://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/

http://stackoverflow.com/questions/12302057/c11-safe-double-checked-locking-for-lazy-initialization-possible/12302355#12302355

[Peter Steinbach]

I have to admit, I am not quite sure I grasp what you are after ... what I saw in your code is this:

static FFTRepo& getInstance( ) {
    static FFTRepo fftRepo;
    return fftRepo;
  };

whereas (as far as I could tell) all other member functions of fftRepo are protected by a pthread lock [specific for *NIX platforms] (not a lock using volatile which would be a bit more cross-platform AFAIK). The function quoted above already is not thread safe. Even if fftRepo is initialized thread-safe, what guarantee do you have that any access between the initialization and the return jump is in the same thread? your current thread could be halted and another one jumps in.

PS. C++AMP is based on C++2003 (correct me if I am wrong, I know it offers lambda support, but C++11 is more than lambdas, so I am not sure what standard it's based on). If it is based on c++11, consider using the C++11 mutex classes or std::atomic.

[Neelakandan Ramachandran]

My point is even if another thread jumps in, it would not cause a reinitialization of fftrepo being static in nature. The halted thread shall return gracefully when it gets its turn( time depends on scheduler) returning a valid singleton object right ?.

[Peter Steinbach]

true, but will it be in the right state for its consumer? thread A wants to aquire a plan for 1024 say, but thread B jumps in and deletes it while thread A is on halt, then thread A comes back, gets access to the repo and calls getPlan for 1024 on the retrieved singleton. nothing will be there because thread B has deleted it meanwhile.

[Neelakandan Ramachandran]

Well I mistook the race condition involved is in the creation of singleton object. But what you illustrate here racing condition arising in the usage (member functions ) of this singleton object. Let me think loud to get better grip on this

[Neelakandan Ramachandran]

Not able to visualize how atomic or mutex will ensure thread safety across member functions. Lets say thread A atomically creates a plan and succeeds. [Context swtich 1]: Thread B now could atomically delete the plan created by A. [Context Switch] Thread A in its turn would expect the existence of plan but its now atomically destroyed :-) . How to ensure thread safety across all the member functions ?

[Peter Steinbach]

ok, let's backtrack ... I have the feeling we are not on the same page: you are using which standard to compile hcfft?

I called make VERBOSE=1 in the hcfft build directory and saw this: /opt/rocm/hcc/bin/clang++ -fPIC -hc -std=c++amp, but you are linking with -stdlib=libc++ which indicates C++11 and above. btw, would it work to compile hcfft with -std=c++1x with x={1,4,y}?

I am asking because I was under the impression that you are using the C++2003 standard (also due to your custom pthread mutex class). If that is not the case, and you are using C++2011 and above, I can see that the singleton impletation makes sense. Please clearify.