Adios2: Race condition in SST engine, resulting in rare segfaults

[franzpoeschel]

Describe the bug
When testing streaming via ADIOS2's SST engine in the openPMD API, I noticed that one of my tests would seldomly crash with a segfault. This test opens an SST reader and an SST writer concurrently as two threads in the same process.

To Reproduce
I tried to build a similar example using only ADIOS2 and was able to reproduce the random segfaulting with a small example:

#include <adios2.h>
#include <array>
#include <future>
#include <iostream>
#include <numeric>
#include <thread>
#include <condition_variable>
#include <shared_mutex>

using dt = long long;

struct synchronize
{
    std::condition_variable_any cond;
    std::shared_mutex mutex;
    bool start = false;
};

void
read( synchronize & s )
{
    adios2::ADIOS adios;
    adios2::IO bpIO = adios.DeclareIO( "IO" );
    bpIO.SetEngine( "sst" );

    // {
    //     std::shared_lock lk( s.mutex );
    //     s.cond.wait( lk, [ &s ]() { return s.start; } );
    // }
    adios2::Engine bpReader = bpIO.Open( "communicate", adios2::Mode::Read );

    std::array< dt, 100000 > ar;

    auto status = bpReader.BeginStep();
    if( status == adios2::StepStatus::EndOfStream )
    {
        return;
    }

    adios2::Variable< dt > var = bpIO.InquireVariable< dt >( "data" );
    bpReader.Get( var, ar.begin() );
    bpReader.EndStep();
    for( auto val : ar )
    {
        // std::cout << val << ", ";
    }
    bpReader.Close();
}


void
write( synchronize & s )
{
    adios2::ADIOS adios;
    adios2::IO bpIO = adios.DeclareIO( "IO" );
    bpIO.SetEngine( "sst" );
    // {
    //     std::shared_lock lk( s.mutex );
    //     s.cond.wait( lk, [ &s ]() { return s.start; } );
    // }
    adios2::Engine bpWriter = bpIO.Open( "communicate", adios2::Mode::Write );

    auto var = bpIO.DefineVariable< dt >(
        "data",
        adios2::Dims{ 10000, 10 },
        adios2::Dims{ 0, 0 },
        adios2::Dims{ 10000, 10 } );

    std::array< dt, 100000 > ar;

    std::iota( ar.begin(), ar.end(), 0 );

    bpWriter.BeginStep();
    bpWriter.Put< dt >( var, ar.begin() );
    bpWriter.EndStep();
    bpWriter.Close();
}

int
main()
{
    synchronize s;
    auto read_fut = std::async( std::launch::async, read, std::ref( s ) );
    auto write_fut = std::async( std::launch::async, write, std::ref( s ) );
    // std::this_thread::sleep_for( std::chrono::milliseconds( 200 ) );
    // {
    //     std::lock_guard lk( s.mutex );
    //     s.start = true;
    // }
    s.cond.notify_all();
    read_fut.wait();
    write_fut.wait();
}

(I tried including some locking mechanisms to trigger the issue more reliably, but when commenting those pieces in, I wasn't able to reproduce the issue at all. Left it in for now since it may or may not be helpful in chasing down the issue. Requires C++17 to build.)
CMakeLists.txt:

cmake_minimum_required(VERSION 3.1)
project(adios2_test)
find_package(ADIOS2 REQUIRED)
add_executable(concurrent concurrent.cpp)
set_target_properties(concurrent PROPERTIES CXX_STANDARD 17)
target_link_libraries(concurrent adios2::adios2)

Concurrency being concurrency, this issue is not always triggered. Sample output:

> i=0; while true; do echo "$((i++))"; ./concurrent || break; done
0
1
2
3
4
[…]
29
30
31
Segmentation fault (core dumped)

Expected behavior
Should not crash. I am not sure whether this can also be triggered when communicating between different processes. If not, this might be also wrong usage of the SST engine.

Desktop (please complete the following information):

• Platform: Ubuntu 20.04, Intel Core i5-6200U,
• Build g++ 9.3.0, dynamic build, latest master (130c4f14)

[eisenhauer]

Thanks Franz. While thread-to-thread streaming within a process isn't a normal use case for us, you're right that this shouldn't die like this. I'll take a look and see what's going on...

[eisenhauer]

Some relevant notes here... There were thread-related decisions made early on in the design of ADIOS2. In particular, it was decided that ADIOS2 would not have a thread-safe API, and that we'd assume that all user-facing ADIOS routines would be called from the main program thread. Mostly these decisions were made so that we could easily operate on top of the common, non-thread-safe, MPI implementations, but SST relies upon those assumptions in a number of places. When I run your code, I get a segfault in SST at a point where we're creating a static data structure to be shared across all SST engines in a process. Because of the assumption that all engine creation is done by the main program thread, we assume that these are serialized and don't protect that structure with a mutex.

So, your test code does fall outside our design assumptions and uses that we officially support. However, presumably you're doing this for a reason and it's useful to you, so I'd like to make it work if I can do so without adding a lot of complexity and/or affecting performance in some way. At least this particular circumstance I can fix up pretty easily. What I don't know is how many other places there are where we're going to run into similar problems. I'll do what is tractable, but I wanted to fill you in on the situation...

[franzpoeschel]

Alright, that's good to know – and thank you for investigating!
If, as I'm understanding, this is actually an issue that cannot arise when using the SST engine from different processes, this is fine for my use cases and the easiest solution would probably be for me to come up with a different way of testing things in the openPMD API.

Maybe I missed it, but I would suggest documenting that the SST engine cannot safely be used for communication within one process (and maybe also emphasize in the Advice section under the "thread safety" item that thread-unsafety also applies to fully unrelated objects).

[eisenhauer]

Agree that the Advice section's "use a mutex" is unclear. The difficulty is that making it clearer is likely to be problematic. We have no tests that exercise thread-based usage of ADIOS because we're pretty much stuck to the use "from the master thread" bit of the advice. I know that SST has internal resources that are shared between engines which would potentially problematic if you just want to segregate user-level items by thread as a means of concurrency control. I don't know about the other engines...

[franzpoeschel]

Maybe a little note in the SST documentation that it is intended for communication between different processes could be useful? But that's up to you, from my side, this issue can be closed.
Thanks for the quick help again!

[eisenhauer]

I'm preparing a PR that protects SST's shared state in this circumstance (it isn't extensive) and have a modified version of your test that can work as a regression test for our engines. Once I get it done, I'll see if there are other engines with problems. But yes, in general the ADIOS staging engines are not intended for intra-process communication. The exception to that is the inline engine, which is a different sort of beast...

[franzpoeschel]

Sounds great!

[eisenhauer]

PR #2334, just merged with master, supports this thread-to-thread use case. (Beyond testing, whether or not it makes sense to use a socket or RDMA-based transport for thread-to-thread communication is another matter!)