GH-41004: [C++][FS][Azure] Don't run TestGetFileInfoGenerator() with Valgrind

[kou]

Rationale for this change

GetFileInfo() with generator reports false positive memory leak in Azure SDK for C++.

What changes are included in this PR?

Don't run TestGetFileInfoGenerator() with Valgrind.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

• GitHub Issue: [CI][C++] CPP valgrind job is failing on Azure tests #41004

[kou]

@github-actions crossbow submit test-conda-cpp-valgrind

[github-actions]

⚠️ GitHub issue #41004 has been automatically assigned in GitHub to PR creator.

[github-actions]

Revision: 5f4255f

Submitted crossbow builds: ursacomputing/crossbow @ actions-29b32e4857

Task	Status
test-conda-cpp-valgrind

[kou]

+1

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6e8ac43.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 20 possible false positives for unstable benchmarks that are known to sometimes produce them.

[pitrou]

We already have ARROW_VALGRIND, I don't understand why we would need to introduce this new constant.

[kou]

Oh, sorry. I missed it. Can we move ARROW_VALGRIND to config.h.cmake from add_definitions(-DARROW_VALGRIND)?

[pitrou]

This is not an information that is useful to third-party code, so I don't understand why that would be useful.

[kou]

This will not be useful to third-party code but it helps us to know what macros are defined in a build.
If we use add_definitions(-DARROW_VALGRIND), we need to use ninja -v and find a -DARROW_VALGRIND from a long command line.

[kou]

#41176/#41177

[pitrou]

@kou Did we try to understand why a memory leak was reported? We shouldn't just silence the errors that annoy us.

cc @felipecrv

[kou]

My understanding is https://github.com/apache/arrow/pull/40567/files#diff-7d2cbf9c6abf1ee980b8cf5a79e222543c17b3d93b12c4c0c9fa123b1bb200b6R398-R411 .

[pitrou]

Ok... This seems to assume that Valgrind is buggy and doesn't handle a calloc() call correctly, while our Azure FS implementation (and/or the Azure SDK) is spotless and cannot contain any bugs. I'm quite skeptical.

There are couple search results online for this, but very few, so I'm not sure it's really an issue in libxml or in the Azure SDK. Have you tried opening a bug report there? Perhaps they know more about the issue.

Really, deciding that errors are "false positives" without any further analysis does not strike me as a serious way to deal with CI issues.

And even if we decide it's a false positive, we can add a Valgrind suppression for the case of xmlNewGlobalState, which would still help us find other issues:
https://github.com/apache/arrow/blob/main/cpp/valgrind.supp

[felipecrv]

Ok... This seems to assume that Valgrind is buggy and doesn't handle a calloc() call correctly, while our Azure FS implementation (and/or the Azure SDK) is spotless and cannot contain any bugs

The reason I didn't make progress on the issue was also this sense that this seems to be an actual leak and not a misreport that we can just suppress.

But I suspect it's something in the Azure SDK (exception-safety failure?) because GetFileInfoWithSelectorFromContainer doesn't do complicated memory management. For instance, the lambdas don't even hold shared/strong references to anything.

arrow/cpp/src/arrow/filesystem/azurefs.cc

Lines 1683 to 1687 in 48a9639

	Status GetFileInfoWithSelectorFromContainer(
	const Blobs::BlobContainerClient& container_client, const Core::Context& context,
	Azure::Nullable<int32_t> page_size_hint, const FileSelector& select,
	FileInfoVector* acc_results) {
	ARROW_ASSIGN_OR_RAISE(auto base_location, AzureLocation::FromString(select.base_dir));

[pitrou]

But I suspect it's something in the Azure SDK (exception-safety failure?) because GetFileInfoWithSelectorFromContainer doesn't do complicated memory management.

That may definitely be possible (in which case the issue should be reported upstream nevertheless), but it would be nice to investigate a bit to make sure we're not missing anything on our side.

[pitrou]

Note that xmlNewGlobalState seems to be some kind of legacy function ("backwards compatibility
of libxml2 to pre-thread-safe behaviour"):
https://gitlab.gnome.org/GNOME/libxml2/-/blob/master/globals.c#L837-845

We see this in xmlGetThreadLocalStorage: xmlNewGlobalState is only called if the macro USE_TLS is not defined.
https://gitlab.gnome.org/GNOME/libxml2/-/blob/master/globals.c#L872-895

USE_TLS in turn gets conditionally defined here:
https://gitlab.gnome.org/GNOME/libxml2/-/blob/master/globals.c#L99-114

Where does libxml2 come in our case? Is it vendored by the Azure SDK? Something else?

[pitrou]

Where does libxml2 come in our case? Is it vendored by the Azure SDK? Something else?

Answering myself: it's version 2.12.6 from conda-forge.

[felipecrv]

And the Azure SDK depends on it:

_deps/azure_sdk-build/sdk/storage/azure-storage-common/azure-storage-common-cppConfig.cmake
35:  find_dependency(LibXml2)

[pitrou]

Judging by the disassembly of xmlGetThreadLocalStorage on conda-forge's libxml2, it seems that USE_TLS doesn't get enabled in that package.

[pitrou]

Ok, that's because --with-tls doesn't get passed here:
https://github.com/conda-forge/libxml2-feedstock/blob/main/recipe/build.sh

[pitrou]

I've reported conda-forge/libxml2-feedstock#117

[kou]

I've also investigated this more.

I think that --with-tls isn't the cause of this.

I could reproduce this by the following program that only uses libxml2 and std::thread:

// g++ -fsanitize=address -g3 -O0 -o aaa aaa.cxx $(pkg-config --cflags --libs libxml-2.0) && ./aaa

#include <chrono>
#include <condition_variable>
#include <mutex>
#include <thread>

#include <libxml/xmlreader.h>

int main(void) {
  xmlInitParser();
  std::mutex mutex;
  std::condition_variable variable;
  std::thread thread([&] {
    xmlTextReaderPtr reader =
      xmlReaderForMemory("<root/>", 7, nullptr, nullptr, 0);
    xmlFreeTextReader(reader);
    variable.notify_one();
    {
      std::unique_lock<std::mutex> lock(mutex);
      variable.wait(lock);
    }
  });
  {
    std::unique_lock<std::mutex> lock(mutex);
    variable.wait(lock);
  }
  xmlCleanupParser();
  variable.notify_one();
  thread.join();
  return 0;
}

The point of this program is that a thread that uses libxml2 API is finished after xmlCleanupParser() is called.

This is also happen in our test. A thread created by arrow::internal::ThreadPool is finished after a xmlCleanupParser() call that is caused in Azure SDK for C++. ( https://github.com/Azure/azure-sdk-for-cpp/blob/067d6acb3b2d1f82b5ad9d258050bc525941d501/sdk/storage/azure-storage-common/src/xml_wrapper.cpp#L398 )

Note that both of them are happen by a C++ destructor. They are called on process exit. Azure::Storage::_internal::XmlGlobalInitializer::~XmlGlobalInitializer() is called before arrow::internal::ThreadPool::~ThreadPool of arrow::io::default_io_context().

If we shutdown a thread of arrow::internal::ThreadPool explicitly before Azure::Storage::_internal::XmlGlobalInitializer::~XmlGlobalInitializer(), the leak isn't reported. For example:

diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc
index ed09bfc2fa..d87e3e0731 100644
--- a/cpp/src/arrow/filesystem/azurefs_test.cc
+++ b/cpp/src/arrow/filesystem/azurefs_test.cc
@@ -58,6 +58,7 @@
 #include "arrow/util/logging.h"
 #include "arrow/util/pcg_random.h"
 #include "arrow/util/string.h"
+#include "arrow/util/thread_pool.h"
 #include "arrow/util/unreachable.h"
 #include "arrow/util/value_parsing.h"
 
@@ -371,6 +372,8 @@ class TestGeneric : public ::testing::Test, public GenericFileSystemTest {
     if (azure_fs_) {
       ASSERT_OK(azure_fs_->DeleteDir(container_name_));
     }
+    // Dirty
+    ASSERT_OK(reinterpret_cast<::arrow::internal::ThreadPool *>(io_context_->executor())->Shutdown());
   }
 
  protected:
@@ -379,7 +382,8 @@ class TestGeneric : public ::testing::Test, public GenericFileSystemTest {
     random::pcg32_fast rng((std::random_device()()));
     container_name_ = PreexistingData::RandomContainerName(rng);
     ASSERT_OK_AND_ASSIGN(auto options, MakeOptions(env_));
-    ASSERT_OK_AND_ASSIGN(azure_fs_, AzureFileSystem::Make(options));
+    io_context_ = std::make_unique<io::IOContext>();
+    ASSERT_OK_AND_ASSIGN(azure_fs_, AzureFileSystem::Make(options, *io_context_));
     ASSERT_OK(azure_fs_->CreateDir(container_name_, true));
     fs_ = std::make_shared<SubTreeFileSystem>(container_name_, azure_fs_);
   }
@@ -417,6 +421,7 @@ class TestGeneric : public ::testing::Test, public GenericFileSystemTest {
 
  private:
   std::string container_name_;
+  std::unique_ptr<io::IOContext> io_context_;
 };
 
 class TestAzuriteGeneric : public TestGeneric {

[pitrou]

I think that --with-tls isn't the cause of this.

Well, we can wait for an updated conda-forge package to make sure that's the case.

[felipecrv]

Looks like yet another case of a library polluting the thread-local storage area :)

[pitrou]

It's more a case of DLL finalization coming before whatever unit of work is still being executed in some worker thread, AFAIU. We had a similar problem with S3 (but resulting in worse misbehavior aka crashes) and had to jump through hoops to (hopefully) solve the problem.

[kou]

It seems that --with-tls is related: #41279 (comment)