Skip to content

RAII wrapper for init and shutdown. #478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

RAII wrapper for init and shutdown. #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions aws-cpp-sdk-cognitoidentity-integration-tests/RunTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,9 @@ int main(int argc, char** argv)
}

options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Trace;
Aws::InitAPI(options);
Aws::APIWrapper api(options);
::testing::InitGoogleTest(&argc, argv);
int exitCode = RUN_ALL_TESTS();
Aws::ShutdownAPI(options);


Aws::Testing::ShutdownPlatformTest(options);
return exitCode;
Expand Down
17 changes: 9 additions & 8 deletions aws-cpp-sdk-core-tests/RunTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@ int main(int argc, char** argv)
Aws::Testing::InitPlatformTest(options);

options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Trace;

Aws::InitAPI(options);
::testing::InitGoogleTest(&argc, argv);
int retVal = RUN_ALL_TESTS();
Aws::ShutdownAPI(options);
EXPECT_EQ(memorySystem.GetCurrentOutstandingAllocations(), 0ULL);
EXPECT_EQ(memorySystem.GetCurrentBytesAllocated(), 0ULL);
EXPECT_TRUE(memorySystem.IsClean());
int retVal = 0;
{
Aws::APIWrapper wrapper(options);
::testing::InitGoogleTest(&argc, argv);
retVal = RUN_ALL_TESTS();
}
EXPECT_EQ(memorySystem.GetCurrentOutstandingAllocations(), 0ULL);
EXPECT_EQ(memorySystem.GetCurrentBytesAllocated(), 0ULL);
EXPECT_TRUE(memorySystem.IsClean());

Aws::Testing::ShutdownPlatformTest(options);

Expand Down
33 changes: 33 additions & 0 deletions aws-cpp-sdk-core/include/aws/core/Aws.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
#include <aws/core/http/HttpClientFactory.h>
#include <aws/core/Core_EXPORTS.h>

#include <atomic>
#include <mutex>

namespace Aws
{
static const char* DEFAULT_LOG_PREFIX = "aws_sdk_";
Expand Down Expand Up @@ -238,5 +241,35 @@ namespace Aws
* Do not call any other SDK methods after calling ShutdownAPI.
*/
AWS_CORE_API void ShutdownAPI(const SDKOptions& options);

/**
* Ref counted RAII wrapper around SDK Init and Shutdown. The first one created will call InitAPI() and the last one to go out of scope
* will call ShutdownAPI(). This is intended for use cases, such as plugin frameworks where multiple modules depending on the SDK may be loaded
* without being aware of one another. This allows each module to safely init/shutdown the SDK as long as each module uses this wrapper.
*
* Note: whichever instance of SDKOptions is used to init the API, will be used for Shutdown(), and InitAPI will only be called with the first
* call to the Constructor. All other values will be ignored.
*/
class AWS_CORE_API APIWrapper
{
public:
/**
* Initialize the SDK if it hasn't been already. Otherwise just increase the ref count.
*/
APIWrapper(const SDKOptions& options);
/**
* Shutsdown the SDK if no other instances of this class exist. Otherwise just decreases the ref count.
*/
~APIWrapper();

APIWrapper(const APIWrapper&) = delete;
APIWrapper& operator=(const APIWrapper) = delete;

private:
static std::atomic<unsigned> RefCount;
static std::mutex InitGuard;

SDKOptions m_sdkOptions;
};
}

41 changes: 41 additions & 0 deletions aws-cpp-sdk-core/source/Aws.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,45 @@ namespace Aws
}
#endif // USE_AWS_MEMORY_MANAGEMENT
}

std::atomic<unsigned> APIWrapper::RefCount = 0;
std::mutex APIWrapper::InitGuard;

APIWrapper::APIWrapper(const SDKOptions& options)
{
if (RefCount.load() == 0u)
Copy link
Contributor

@diablodale diablodale Aug 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. I think the separated if test outside the mutex makes the logic not thread-safe. I think a fail case is the following. This is on some computer with 8 CPU cores and a multithreading OS capable of simultaneous thread execution + time-slicing.

  1. Two threads have previously run the constructor. Therefore, the value of RefCount = 2.
  2. Thread 1 runs the destructor. On line 140, it evals to false because the value of RefCount is 2. Therefore, this thread will run the else clause sometime in the future.
  3. Thread 2 runs the destructor. On line 140, it evals to false because the value of RefCount is 2. Therefore, this thread will run the else clause sometime in the future.
  4. Thread 1 runs line 152 and decrements RefCount. Its value is now 1.
  5. Thread 2 runs line 152 and decrements RefCount. Its value is now 0.

This results in no thread calling Aws::ShutdownAPI(). You can also run the above in parallel by steps 2 and 3 running at exactly the same time. And steps 4 and 5 running at the same time. The value in RefCount will not be corrupted because it is atomic. However, the if-test was not atomic/protected and therefore fails these scenarios.

So what if we moved to use only one if test and the atomic.fetch_add/sub()? Here is sample code:

std::atomic<size_t> APIWrapper::s_count{ 0 };
std::mutex APIWrapper::s_initGuard{};
Aws::SDKOptions APIWrapper::s_awsOptions{};

APIWrapper::APIWrapper() {
    const size_t origCount = s_count.fetch_add(1);
    std::lock_guard<std::mutex> lock(s_initGuard);
    if (0 == origCount) {
        Aws::InitAPI(s_awsOptions);
    }    
}

APIWrapper::~APIWrapper() {
    const size_t origCount = s_count.fetch_sub(1);
    std::lock_guard<std::mutex> lock(s_initGuard);
    if (1 == origCount) {
        Aws::ShutdownAPI(s_awsOptions);
    }
}

This will also fail and is not thread safe. Here is one fail scenario:

  1. Two threads have previously run the constructor. Therefore, the value of RefCount = 2 and InitApi has already been called.
  2. Thread 1 runs the destructor first line but not yet lock_guard. origCount value is 2. RefCount=1.
  3. Thread 2 runs the destructor first line but not yet lock_guard. origCount value is 1. RefCount=0.
  4. Thread 3 runs the entire constructor. origCount value is 0. RefCount=1. This causes InitAPI() to be called when it have already been called previously. Unknown affect.
  5. Thread 2 evals (1 == origCount) as true. Therefore it runs the main clause and calls Shutdown
  6. Thread 3 tries to call AWS apis and crashes due to calling apis after Shutdown.

I believe what is needed is the following:

  1. an atomic/protected variable to hold the RefCount
  2. for a given thread, an atomic load-compare of RefCount to see if this thread needs to Init/Shutdown
  3. when the thread in (2) determines it needs to do an Init/Shutdown, that no other thread will do an Init/Shutdown
  4. when the thread in (2) determines it needs to do an Init/Shutdown, that no other thread will change the value of RefCount. Why? Because if RefCount changes, that means the need to run Init/Shutdown might change.

Given these, I believe a mutex is needed around the: increment/decrement, the test, and the Init/Shutdown. I don't think it is possible to protect only a subset of them. Since they all need to be protected, a mutex and lock guard can work well. And using this, it is no longer necessary to use an atomic for RefCount as this protection would be redundant since it is surrounded by a mutex.

I would recommend code similar to this:

long int APIWrapper::s_count{ 0 };  // for a natural int length and signed
std::mutex APIWrapper::s_initGuard{};
Aws::SDKOptions APIWrapper::s_awsOptions{};

APIWrapper::APIWrapper() {
    std::lock_guard<std::mutex> lock(s_initGuard);
    s_count++;
    if (1 == s_count) {
        Aws::InitAPI(s_awsOptions);
    }    
}

APIWrapper::~APIWrapper() {
    std::lock_guard<std::mutex> lock(s_initGuard);
    s_count--;
    if (0 == s_count) {
        Aws::ShutdownAPI(s_awsOptions);
    }
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think s_count needs modulo arithmetic so it shouldn't be unsigned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK w/ me. I'm neutral on sign/unsigned in this usage. However, your post made me think again on size_t. I no longer think size_t is stylistically the right type/macro to use. size_t should only be used to represent the size of something (usually in bytes).
Personally, I tend to use unsigned to store ref counts. It is probably because Windows COM objects must use unsigned longs for reference counting, so I'm biased that direction.
I would prefer it be a 32 or 64-wide; tending towards the natural width of an integer on a CPU.
Perhaps long int?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated my suggested code to use long int and clarified two steps in a fail scenario

{
std::lock_guard<std::mutex> locker(InitGuard);

if (RefCount.load() == 0u)
{
m_sdkOptions = options;
Aws::InitAPI(m_sdkOptions);
}

RefCount++;
}
else
{
RefCount++;
}
}

APIWrapper::~APIWrapper()
{
if (RefCount.load() == 1u)
{
std::lock_guard<std::mutex> locker(InitGuard);

if (RefCount.load() == 1u)
{
Aws::ShutdownAPI(m_sdkOptions);
}
RefCount--;
}
else
{
RefCount--;
}
}
}
4 changes: 2 additions & 2 deletions aws-cpp-sdk-dynamodb-integration-tests/RunTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ int main(int argc, char** argv)
Aws::Testing::SetAwsResourcePrefix(argv[1]);
}

Aws::InitAPI(options);
Aws::APIWrapper api(options);

::testing::InitGoogleTest(&argc, argv);
int exitCode = RUN_ALL_TESTS();
Aws::ShutdownAPI(options);

Aws::Testing::ShutdownPlatformTest(options);
return exitCode;
Expand Down
3 changes: 1 addition & 2 deletions aws-cpp-sdk-s3-integration-tests/RunTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ int main(int argc, char** argv)
Aws::Testing::SetAwsResourcePrefix(argv[1]);
}

Aws::InitAPI(options);
Aws::APIWrapper api(options);
::testing::InitGoogleTest(&argc, argv);
int exitCode = RUN_ALL_TESTS();
Aws::ShutdownAPI(options);

Aws::Testing::ShutdownPlatformTest(options);
return exitCode;
Expand Down
3 changes: 1 addition & 2 deletions aws-cpp-sdk-sqs-integration-tests/RunTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@ int main(int argc, char** argv)
}

options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Trace;
Aws::InitAPI(options);
Aws::APIWrapper api(options);
::testing::InitGoogleTest(&argc, argv);
int exitCode = RUN_ALL_TESTS();
Aws::ShutdownAPI(options);

Aws::Testing::ShutdownPlatformTest(options);
return exitCode;
Expand Down
3 changes: 1 addition & 2 deletions aws-cpp-sdk-transfer-tests/RunTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@ int main(int argc, char** argv)
}

options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Trace;
Aws::InitAPI(options);
Aws::APIWrapper api(options);
::testing::InitGoogleTest(&argc, argv);
int exitCode = RUN_ALL_TESTS();
Aws::ShutdownAPI(options);

Aws::Testing::ShutdownPlatformTest(options);
return exitCode;
Expand Down