Test issue after upgrade on Windows

[lordgamez]

Hi everyone,

We have been using the google-cloud-cpp library for a while for managing google cloud storage (uploading, deleting, fetching objects and listing buckets). We have some unit tests set up for these operations using gcs::testing::MockClient (where gcs = ::google::cloud::storage) and creating the gcs::Client in the tests using gcs::testing::ClientFromMock. Previously we were using version 1.37.0 and now we are trying to upgrade to version 2.10.1. After fixing the breaking changes in the tests, everything worked fine on Linux and Mac. The only problem appeared on Windows where an SEH exception was thrown for every test using the gcs::Client. On Windows we are using MSVC with Visual Studio 2019.

After checking the issue with a debugger it seems that the issue comes from the CurrentOptions() method which seems to return an empty thread_local variable.

Exception thrown: read access violation.
std::shared_ptr<google::cloud::v2_10_1::internal::TraitBasedRetryPolicy<google::cloud::storage::v2_10_1::internal::StatusTraits> >::operator->**<google::cloud::v2_10_1::internal::TraitBasedRetryPolicy<google::cloud::storage::v2_10_1::internal::StatusTraits>,0>**(...) returned nullptr.

PutGCSObjectTests.exe!google::cloud::storage::v2_10_1::internal::RetryClient::current_retry_policy() Line 943	C++
PutGCSObjectTests.exe!google::cloud::storage::v2_10_1::internal::RetryClient::CreateResumableUpload(const google::cloud::storage::v2_10_1::internal::ResumableUploadRequest & request) Line 433	C++
minifi-gcp.dll!google::cloud::storage::v2_10_1::internal::CreateOrResume(google::cloud::storage::v2_10_1::internal::RawClient & client, const google::cloud::storage::v2_10_1::internal::ResumableUploadRequest & request) Line 27	C++
minifi-gcp.dll!google::cloud::storage::v2_10_1::Client::WriteObjectImpl(const google::cloud::storage::v2_10_1::internal::ResumableUploadRequest & request) Line 98	C++
minifi-gcp.dll!google::cloud::storage::v2_10_1::Client::WriteObject<google::cloud::storage::v2_10_1::MD5HashValue &,google::cloud::storage::v2_10_1::Crc32cChecksumValue &,google::cloud::storage::v2_10_1::EncryptionKey &,google::cloud::storage::v2_10_1::ContentType &,google::cloud::storage::v2_10_1::PredefinedAcl &,google::cloud::storage::v2_10_1::IfGenerationMatch &>(const std::string & bucket_name, const std::string & object_name, google::cloud::storage::v2_10_1::MD5HashValue & <options_0>, google::cloud::storage::v2_10_1::Crc32cChecksumValue & <options_1>, google::cloud::storage::v2_10_1::EncryptionKey & <options_2>, google::cloud::storage::v2_10_1::ContentType & <options_3>, google::cloud::storage::v2_10_1::PredefinedAcl & <options_4>, google::cloud::storage::v2_10_1::IfGenerationMatch & <options_5>) Line 1186	C++
minifi-gcp.dll!org::apache::nifi::minifi::extensions::gcp::`anonymous namespace'::UploadToGCSCallback::operator()(const std::shared_ptr<org::apache::nifi::minifi::io::InputStream> & stream) Line 47	C++

The originating function in options.cc:

Options& ThreadLocalOptions() {
  thread_local Options current_options;
  return current_options;
}

We are building a separate shared client library as a google-cloud plugin in our application and the google-cloud-cpp library is built locally as a static library. The google cloud library is then linked to the client plugin dll. The client code in the dll looks something like this:

class Base {
 public:
  virtual gcs::Client getClient() const {
    auto options = gcs::ClientOptions(gcp_credentials_);
    return gcs::Client(options, *retry_policy_);
  }
  std::shared_ptr<google::cloud::storage::oauth2::Credentials> gcp_credentials_;
  google::cloud::storage::RetryPolicyOption::Type retry_policy_ = std::make_shared<google::cloud::storage::LimitedErrorCountRetryPolicy>(6);
};

class A : public Base {
 public:
  void foo() {
    auto client = getClient();
    client.WriteObject("my-bucket-name", "my-object-name");
  }
};

The getClient is overriden in the unit tests and the client library dll is linked to the test executables. The test looks something like this:

class MockA : public A {
 public:
  gcs::Client getClient() const override {
    return gcs::testing::ClientFromMock(mock_client_, *retry_policy_);
  }
  std::shared_ptr<gcs::testing::MockClient> mock_client_ = std::make_shared<gcs::testing::MockClient>();
};

TEST(Testsuite, OwnMockClientTest) {
  std::shared_ptr<A> test_class = std::make_shared<MockA>();
  test_class->foo();
}

What may have changed since the previous version that requires change on Windows?

[dbolduc]

Hey, thanks for the detailed report.

What may have changed since the previous version that requires change on Windows?

In v1.42.0 we changed the implementation for passing configuration options from the client to lower layers. We did this to support per-operation configuration for GCS. The implementation involves thread_local storage, as you point out.

The thread_local Options current_options stores information in a map keyed on typeids:

google-cloud-cpp/google/cloud/options.h

Line 178 in f3eedec

auto const it = m_.find(typeid(T));

Maybe these typeids are different between the library and the application, depending on when/how the plugin is loaded? If so, we would see the get() returning a nullptr.

google-cloud-cpp/google/cloud/storage/internal/retry_client.cc

Line 944 in f3eedec

.get<RetryPolicyOption>()

---

I don't have any help to offer just yet. I see apache/nifi-minifi-cpp#1572 and I am trying to build and run the tests under https://github.com/lordgamez/nifi-minifi-cpp/tree/MINIFICPP-2027/extensions/gcp/tests on Windows to reproduce the problem.

I will keep working on this.

---

The one thing that is strange in the stack trace, is that the execution jumps from the library to the executable. It seems like it should stay in the library.

[lordgamez]

Hi Darren, thanks for the quick reply and for looking into this!

Just a quick overview of the gcp library build in minifi in case it may help. The google cloud library build is configured with cmake in the cmake/GoogleCloudCpp.cmake file as you may have already seen in the PR, then the library is linked to the minifi-gcp extension in the extensions/gcp/CMakeLists.txt file using target_link_libraries(minifi-gcp ${LIBMINIFI} google-cloud-cpp::storage)

The tests are built separately as described in extensions/gcp/tests/CMakeLists.txt where the minifi-gcp shared library is linked with target_link_libraries(${testfilename} minifi-gcp). This is where the gtest_main and gmock libraries are also linked with target_link_libraries(${testfilename} gtest_main gmock)

When the test is started most test suites include the libminifi/test/TestBase.h where there is an ExtensionManager object (found in libminifi/src/core/extension/ExtensionManager.cpp). This statically initializes all MiNiFi extensions present. It handles extensions as DynamicLibrary objects (libminifi/src/core/extension/DynamicLibrary.cpp) loading them with dlopen (defined manually for Windows in the same file).

[dbolduc]

Hey, I went on a vacation for a few days, but I am back on the case.

I was able to pull your PR and reproduce the Windows test failures. For maintainers of google-cloud-cpp: the google::cloud::internal::CurrentOptions() are empty.

---

https://learn.microsoft.com/en-us/windows/win32/dlls/using-thread-local-storage-in-a-dynamic-link-library says

"Problems can occur with a process that uses run-time linking because the entry-point function is not called for threads that exist before the LoadLibrary function is called, so TLS memory is not allocated for these threads."

I think that is our problem. And the docs give sample code for how to fix the problem.

I will keep working on this. I think the fix involves changing core-minifi, so you may want to try it yourself. You will probably be faster than me.

[lordgamez]

Hi, thanks for documentation, this looks promising. I will try to implement the provided solution and will get back to you with the result.

[lordgamez]

After checking the example it seems to me that the issue can be fixed solely by implementing the DllMain in the google-cloud-cpp library and initializing the thread_local there without the need to modify core-minifi as there is no need to access the variable explicitly outside the dll. Although I'm not sure where the DllMain should be implemented, and the current_options variable is defined in an anonymous namespace so I am also unsure how that should be made available to be initialized for the DllMain

[dbolduc]

Hey, so I asked for a second opinion and @coryan pointed out that the test executable is both

• linking the minifi-gcp library directly
• calling LoadLibraryEx() on minifi-gcp.dll

This means there are two versions of the libraries symbols. (I did notice that the address of thread_local current_options was different in the test executable and plugin.)

After checking the example it seems to me that the issue can be fixed solely by implementing the DllMain in the google-cloud-cpp library and initializing the thread_local there without the need to modify core-minifi as there is no need to access the variable explicitly outside the dll.

I think I led you astray here. In reading further, I don't think trying to share the TLS across modules on windows is the way forward. Even if it is possible, I think the solution will be very ugly.

---

Q1: Is this gcs::Client GCSProcessor::getClient() a test-only interface? Would consumers of nifi-minifi-cpp override it?

https://github.com/apache/nifi-minifi-cpp/blob/6a4d199394a9efe822bf9e86f2f49526de191dfc/extensions/gcp/processors/GCSProcessor.h#L54

If consumers of nifi-minifi-cpp do override the call, they will run into the same problem we are having with these tests. But if the interface is only for testing, we can probably work around the windows TLS problem.

Q2: Do we have to LoadLibrary() in the tests? Can the tests use the linked library exclusively?

---

Here is a workaround (assuming getClient() is for testing only)....

We added a new interface to create mock clients in #11697. This mock client does not retry on transient errors. It will avoid the code paths that depend on the thread local storage.

To update your code, you would change: gcs::testing::ClientFromMock(mock_client_, *retry_policy_) -> gcs::testing::UndecoratedClientFromMock(mock_client_)

https://github.com/apache/nifi-minifi-cpp/blob/6a4d199394a9efe822bf9e86f2f49526de191dfc/extensions/gcp/tests/DeleteGCSObjectTests.cpp#L42

and tests like this would only expect a single transient error, instead of multiple:
https://github.com/apache/nifi-minifi-cpp/blob/6a4d199394a9efe822bf9e86f2f49526de191dfc/extensions/gcp/tests/DeleteGCSObjectTests.cpp#L90-L91

This test interface will be included in our next release scheduled for 2023-06-01 (give or take a few days)

[lordgamez]

Hi, thanks for the workaround, to answer your questions:

1. gcs::Client GCSProcessor::getClient() is only overriden in tests, no consumers in nifi-minifi-cpp override it
2. Some extensions are optional and have to be loaded manually if we want to use them like the expression-language library. If not loaded manually minifi processor properties are used without expression language support. You are right we should not load everything though, it is just the default behavior in tests that we load every available extension, this can be improved on in the future. I will check if it works in this case if only the required extensions are loaded and we keep the gcp extension only in the linking process.

Having that said, the workaround works, the tests are passing, thank you again!

Only one thing I found problematic is that defining gcs::testing::UndecoratedClientFromMock in the .cc file requires us to build the tests and link the test library, whereas before we only needed the header file. For our use case I created patch for that cmake file to create a separate library that contains only the mock_client.cc source file outside the BUILD_TESTS flag so only that lib has to be built and linked to our tests. In the future it may be better to have a separate library containing this with other test utilities.

[lordgamez]

I quickly checked and after removing minifi-gcp library from the extension list used for the LoadLibrary() calls the issue persists, so I will keep the suggested workaround for now.

[dbolduc]

the workaround works, the tests are passing

yay!

Only one thing I found problematic is that defining gcs::testing::UndecoratedClientFromMock in the .cc file requires us to build the tests and link the test library, whereas before we only needed the header file.

I sent #11708 to inline the function in the header. Hopefully that lets you remove your workaround-within-a-workaround. Thanks for the feedback!

In the future it may be better to have a separate library containing this with other test utilities.

Yeah... You are right. We want to do this and the work is tracked in #7142. While it is an important problem, it is also a very hard problem. I can provide no timeline for when or if this work will get done.

[lordgamez]

Thank you for addressing this, it's not a big problem for now, so I'm okay with having no timeline for it, I will subscribe to the issue to keep track.