[BugFix][C++] Finalize S3 in FileSystem destructor (#289)

apache · Dec 1, 2023 · dcf4bd6 · dcf4bd6
1 parent 536c44d
commit dcf4bd6
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 4 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -107,6 +107,7 @@ jobs:
     - name: Test
       run: |
         cd build
+        export ASAN_OPTIONS=detect_leaks=0
         export GAR_TEST_DATA=$PWD/../testing/
         make test
 

diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
@@ -169,6 +169,7 @@ if (NOT Arrow_FOUND OR NOT ArrowDataset_FOUND OR NOT ArrowAcero_FOUND OR NOT Par
     message(STATUS "Arrow is not installed, will build from source.")
     set(BUILD_ARROW_FROM_SOURCE ON)
 else()
+    message(STATUS "-- Found Arrow: ${Arrow_VERSION}")
     set(BUILD_ARROW_FROM_SOURCE OFF)
 endif()
 

diff --git a/cpp/include/gar/util/filesystem.h b/cpp/include/gar/util/filesystem.h
@@ -59,7 +59,7 @@ class FileSystem {
   explicit FileSystem(std::shared_ptr<arrow::fs::FileSystem> arrow_fs)
       : arrow_fs_(arrow_fs) {}
 
-  ~FileSystem() = default;
+  ~FileSystem();
 
   /**
    * @brief Read and filter a file as an arrow::Table.

diff --git a/cpp/src/filesystem.cc b/cpp/src/filesystem.cc
@@ -18,6 +18,7 @@ limitations under the License.
 #include "arrow/csv/api.h"
 #include "arrow/dataset/api.h"
 #include "arrow/filesystem/api.h"
+#include "arrow/filesystem/s3fs.h"
 #include "arrow/ipc/writer.h"
 #include "parquet/arrow/writer.h"
 
@@ -267,6 +268,13 @@ Result<IdType> FileSystem::GetFileNumOfDir(const std::string& dir_path,
   return static_cast<IdType>(file_infos.size());
 }
 
+FileSystem::~FileSystem() {
+#if defined(ARROW_VERSION) && ARROW_VERSION >= 12000000
+  // Finalize the S3 client if it is initialized, otherwise it would mutex error
+  arrow::fs::FinalizeS3();
+#endif
+}
+
 Result<std::shared_ptr<FileSystem>> FileSystemFromUriOrPath(
     const std::string& uri_string, std::string* out_path) {
   if (uri_string.length() >= 1 && uri_string[0] == '/') {

diff --git a/cpp/test/test_info.cc b/cpp/test/test_info.cc
@@ -367,8 +367,6 @@ TEST_CASE("test_graph_info_load_from_file") {
   REQUIRE(edge_infos.size() == 1);
 }
 
-// ISSUE-187
-#if defined(ARROW_VERSION) && ARROW_VERSION < 12000000
 TEST_CASE("test_graph_info_load_from_s3") {
   std::string path =
       "s3://graphar/ldbc/ldbc.graph.yml"
@@ -382,5 +380,4 @@ TEST_CASE("test_graph_info_load_from_s3") {
   REQUIRE(vertex_infos.size() == 8);
   REQUIRE(edge_infos.size() == 23);
 }
-#endif
 }  // namespace GAR_NAMESPACE