[C++] arrow::fs::FileSystemFromUri() not thread-safe with s3 URIs

[frreiss]

Describe the bug, including details regarding any error messages, version, and platform.

On MacOS, with Arrow version 15.0.0 and AWS SDK for C++ version 1.11.250, calling the C++ function arrow::fs::FileSystemFromUri() simultaneously from multiple threads results in a segmentation fault.

Here is a program that reproduces the problem on my machine:

#include <iostream>
#include <thread>
#include <arrow/api.h>
#include <arrow/filesystem/api.h>

void thread_main()
{
    // Call FileSystemFromURI() and check the returned status code.
    std::string path;
    auto result = arrow::fs::FileSystemFromUri("s3://bucket/key", &path);
    if (result.ok()){
        std::cout << std::this_thread::get_id() << ": FileSystemFromUri() succeeded." << std::endl;
    } else {
        std::cout << std::this_thread::get_id() << ": FileSystemFromUri() failed." << std::endl;
    }
}

int main()
{
    const int num_threads = 2;
    auto threads = std::vector<std::thread>();
    for (int i = 0; i < num_threads; i++)
    {
        threads.push_back(std::thread(thread_main));
    }

    for (auto &thread : threads)
    {
        thread.join();
    }

    // This line would prevent a segfault if the above code were to work.
    (void)arrow::fs::FinalizeS3();
}

This program should produce output like:

0x16eeeb000: FileSystemFromUri() succeeded.
0x16b5a7000: FileSystemFromUri() succeeded.

Instead, it experiences a segmentation fault before printing any output.

Component(s)

C++

[kou]

Could you initialize S3 on the main thread explicitly?

#include <iostream>
#include <thread>
#include <arrow/api.h>
#include <arrow/filesystem/api.h>

void thread_main()
{
    // Call FileSystemFromURI() and check the returned status code.
    std::string path;
    auto result = arrow::fs::FileSystemFromUri("s3://bucket/key", &path);
    if (result.ok()){
        std::cout << std::this_thread::get_id() << ": FileSystemFromUri() succeeded." << std::endl;
    } else {
        std::cout << std::this_thread::get_id() << ": FileSystemFromUri() failed." << std::endl;
    }
}

int main()
{
   (void)arrow::fs::EnsureS3Initialized();
   const int num_threads = 2;
    auto threads = std::vector<std::thread>();
    for (int i = 0; i < num_threads; i++)
    {
        threads.push_back(std::thread(thread_main));
    }

    for (auto &thread : threads)
    {
        thread.join();
    }

    // This line would prevent a segfault if the above code were to work.
    (void)arrow::fs::FinalizeS3();
}

[frreiss]

Yes, making a call to EnsureS3Initialized() prior to launching any background threads makes the example work.

It looks like I can call that function from inside the worker threads while holding a mutex and get the same effect:

#include <iostream>
#include <thread>
#include <arrow/api.h>
#include <arrow/filesystem/api.h>

std::mutex s3_init_mutex;

arrow::Status thread_main()
{
    {
       std::lock_guard<std::mutex> guard(s3_init_mutex); 
       ARROW_RETURN_NOT_OK(arrow::fs::EnsureS3Initialized());
    }

    std::string path;
    auto result = arrow::fs::FileSystemFromUri("s3://bucket/key", &path);
    if (result.ok()){
        std::cout << std::this_thread::get_id() << ": FileSystemFromUri() succeeded." << std::endl;
    } else {
        std::cout << std::this_thread::get_id() << ": FileSystemFromUri() failed." << std::endl;
    }
    return result.status();
}

int main()
{
    const int num_threads = 2;
    auto threads = std::vector<std::thread>();
    for (int i = 0; i < num_threads; i++)
    {
        threads.push_back(std::thread(thread_main));
    }

    for (auto &thread : threads)
    {
        thread.join();
    }

    // This line would prevent a segfault if the above code were to work.
    (void)arrow::fs::FinalizeS3();
}

Perhaps FileSystemFromUriReal() should acquire a mutex while calling EnsureS3Initialized()?

[kou]

I think that it's application's responsibility by design:

arrow/cpp/src/arrow/filesystem/s3fs.h

Lines 374 to 384 in 22f2cfd

	/// \brief Shutdown the S3 APIs.
	///
	/// This can wait for some S3 concurrent calls to finish so as to avoid
	/// race conditions.
	/// After this function has been called, all S3 calls will fail with an error.
	///
	/// Calls to InitializeS3() and FinalizeS3() should be serialized by the
	/// application (this also applies to EnsureS3Initialized() and
	/// EnsureS3Finalized()).
	ARROW_EXPORT
	Status FinalizeS3();

@pitrou Do you have any opinion on this?

[pitrou]

We should certainly make EnsureS3Initialized thread-safe (implicitly called by FileSystemFromUri), given that it can be called lazily.

[TangSiyang2001]

I would like to fix it, plz assign it to me.