RAII wrapper for init and shutdown.

[JonathanHenson]

Addresses:

#456
#437

[crusader-mike]

Thank you, Jonathan!

Though, I highly suggest using std::call_once instead of hand-crafted implementation of double-check-locking.

Also, now you'll have to reconcile the situation when two different APIWrapper objects (having two different SDKOptions data) somehow successfully get constructed. So, which options actually are in effect? ;-)

[JonathanHenson]

I don't think there is a way to reconcile that. Any suggestions you have would be welcome. Also, call once doesn't help me on shutdown which has to be called on linux or if custom memory managers are being used.

…

Sent from my iPhone

On Mar 21, 2017, at 5:37 PM, crusader-mike ***@***.***> wrote: Thank you, Jonathan! Though, I highly suggest using std::call_once instead of hand-crafted implementation of double-check-locking. Also, now you'll have to reconcile the situation when two different APIWrapper objects (having two different SDKOptions data) somehow successfully get constructed. So, which options actually are in effect? ;-) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[crusader-mike]

You are correct, I forgot about Shutdown(). Well, in this case your hand-crafted logic is wrong -- you have a at least one race condition in dtor -- if someone else decreases RefCount between your '== 1' comparison and 'RefCount--' -- Shutdown() will never get called. I am pretty sure there could be others -- this idiom is a notorious minefield.

About different options taking effect -- no good suggestions here... :-( You could either forbid it (throw from ctor if new opts != old ones), allow it with indication (new member variable that is true if our opts are ignored) or get rid of opts (move related logic into standalone get/set functions and leave it to end user how to sort out these collisions). Another option would be to make these options local to some context -- i.e. make Init() return some Context structure that has to be passed into every subsequent operation (this also removes need for ref-counting init calls). As you see best variants result in significant change to library design... :-\

Tbh, design of this library is not the ideal one (pls, don't take this as an insult) -- it has "Java" written all over it and uses iostreams (another unfortunately designed lib). Should probably mention very unusual decision to try to avoid exceptions (which makes a lot of things quite awkward).

[JonathanHenson]

"if someone else decrease RefCount between your '== 1' comparison and 'RefCount--' -- Shutdown() will never get called."

I don't see how this is possible given the logic of the constructor. It is possible another RefCount++ comes in, in which case that's a good thing that we skip Shutdown().

But if we are at 1u, we are the last holder of the object, or another object beat us to it. In both cases that's the desired behavior. What am I missing here?

"You could either forbid it (throw from ctor if new opts != old ones)"

Thought about that already. Unfortunately std::function::operator == will always return false if the function is bound. Changing the contract on the init shutdown isn't really an option at this point unfortunately.

Thanks for your comments, and I'm not insulted. Software engineering is usually choosing the least of evils, which means you still have demons floating around and hiding behind design patterns.

"it has "Java" all over it"

are you referring to Outcome and the presence of Getters and Setters on the data objects? I ripped off Outcome from Erlang.

"iostreams "

has an unfortunate reputation ;=) I find it to be quite useful given you are comfortable with implementing std::stream_buf. The intention here is to provide a natural modern C++ interface, which IMHO means pot committing to the C++ standard library--even with all of its warts. This also gives less dependencies and more platforms we can easily support.

"exceptions"

errr. I hear ya. It's not very unusual though. They are turned off in most video game engines (a huge portion of our customer base--which is why we did the whole outcome thing).

Even some places where they aren't turned off RTTI is turned off which forces you to throw heap allocated exceptions all over the place to adequately model the AWS exception hierarchy (throw-back to the hideous MFC exceptions that you had to manually delete of the 90s).

As for the static init and shutdown, there isn't really an option. OpenSSL pollutes itself with global state as does libcurl. They can't be safely used without a mechanism to hook into their init/cleanup functions. Also there has to be a way to inject memory managers, tie them into STL, and then safely clean everything up before the memory manager goes away.

In retrospect, I do regret 2 things. 1st is not doing 99% of this SDK in C99 and then putting a nice little C++ 11 wrapper on top of it for the user-facing SDKs themselves. My second regret is not starting with AsyncIO via. epoll, kpoll, and the async WinHttp implementation (for windows). I've found it impossible to undo that bad design decision and it now has to wait for the next major version.

Eventually we will do a major rev, so if you have suggestions (such as the ones you just offered), we will certainly consider them for the next major version.

[crusader-mike]

I don't see how this is possible given the logic of the constructor.

Race is between two dtors -- think about case when RefCount is 2 and two dtors get called simultaneously.

are you referring to Outcome and the presence of Getters and Setters on the data objects?

Getters and setters is a dead giveaway, yes. But there are other Java-isms too :-) (or should I say "old-style Java-isms"? I have no idea where modern Java is right now). Like multiple interceptions points with factories sprinkled all over and a lot of virtual functions. It is a considerably different design style which (imho!) somehow mostly avoided in C++ world.

I find it to be quite useful given you are comfortable with implementing std::stream_buf

Well, I am not comfortable implementing my own stream_buf, nor I became so after spending few hours of researching this. When I have a chunk of data in memory and Send/Write function takes iostream (not istream!) -- it is quite a pain to write a wrapper that presents my memory buffer as iostream. (or I must be missing something) I ended up using strstream, which is a hack and rightfully produces "deprecated, do not use" warnings.

The intention here is to provide a natural modern C++ interface, which IMHO means pot committing to the C++ standard library--even with all of its warts.

I'd stay away from iostreams, even though they are considered to be part of std library -- it is an ancient evil living in it's own world. (again, imho).

"exceptions"

Yeah... this decision leads to so many problems (like you can't "fail' in constructors, which leads to crazy class invariants). I don't understand fixation of videogames industry on exceptions, tbh. Especially, if library is linked dynamically and it's exported functions don't allow exceptions to escape. Anyways, if we discuss all side effects of this decision -- it'll take a lot of pages.

OpenSSL pollutes itself with global state as does libcurl.

How about putting burden of OpenSSL/libcurl initialization on end-user? Like "before you invoke any AWSCPP function these libs needs to be initialized: ... or behavior is undefined" -- make it user's headache.

(OpenSSL is another source of WTFs... I think someone is rewriting it from scratch right now.)

In retrospect, I do regret 2 things. 1st is not doing 99% of this SDK in C99

Well, nothing prevents you from doing it in C++ and having typical C-style public interface and then wrap it in C++ again. ;-) But seriously -- C++ is a way to go, just design needs to be 'more C++' -- if it is done right, everything will become much simpler.

My second regret is not starting with AsyncIO via. epoll, kpoll, and the async WinHttp implementation (for windows).

Yes. Async programming (esp in cross-platform environment) is very non-trivial. C++ community waits with anticipation for asio to make it into standard. (tbh, I never used it -- my homebrewed tools are fine for my needs).

[JonathanHenson]

"Race is between two dtors -- think about case when RefCount is 2 and two dtors get called simultaneously."

hmmm.... I see it now. I'll just move the mutex up, it's not like it matters unless you are planning on calling these things 100 times per second--haha please don't do that.

"Getters and setters is a dead giveaway, yes"

That's more of an OOP thing than a Java thing, and I think you'd find the setters aren't doing nothing. They are actually managing additional state (which is the point of encapsulation). That's not a "not C++" convention. I've never written a getter/setter in java in my life.

"factories sprinkled all over and a lot of virtual functions. "

How else would you provide cross-platform implementations that are not necessarily related to compile time configurations? Also....I'm pretty sure, this was C++ and Small Talk, not Java:
https://www.amazon.com/Design-Patterns-Elements-Reusable-Object-Oriented/dp/0201633612

Unless you are suggesting TMP, in which case , we might as well just use boost.

I find your belief in the notion of a "C++ style" amusing. "Javaisms" bleh.... I could point you to a long list of C++ style that follows the conventions here. Most things Microsoft distributes for example. Anyways, it is what it is. There is no platonic "C++ Style Guide". There is only safe/unsafe, extendable/brittle, cross-platform/worthless, and efficient/might-as-well be Java or C#.

"Well, nothing prevents you from doing it in C++ and having typical C-style public interface and then wrap it in C++ again. ;-) But seriously -- C++ is a way to go, just design needs to be 'more C++' -- if it is done right, everything will become much simpler."

C gets us more platforms and less fighting with GCC versions/ABIs. Also, the lack of safety mechanisms in C around heap allocated memory force me to have better memory models, because I just avoid allocating memory altogether and force it on the caller. I can also sling it 10x faster than C++.

Anyways, "design needs to be 'more C++' -- if it is done right, everything will become much simpler.". Do you have an example of this? I can try and add it to our deep dive for v2.

Also we should probably take this offline from this PR. If you are in Seattle I can buy you a beer, and we can make this discussion more.... programmer-like.

[crusader-mike]

You maybe right about "Javaisms" -- in my experience I observed that ppl with Java background tend to create designs along similar lines. I never saw Smalltalk code and many other things. Your experience is different, so we have different views on this. It is ok.

Yes, this discussion is better to be taken offline -- I am up for a beer, but alas I am in Houston and will unlikely to show up in Seattle any time soon. Contact me at printf("crusader.%s@%s", "mike", "gmail.com"); to discuss this -- I may come up with something actually useful :-)

[OlafvdSpek]

The wrapper doesn't guarantee ShutdownAPI is called with the same options as InitAPI does it?

auto aws0 = make_unique<Aws::APIWrapper>(options0);
auto aws1 = make_unique<Aws::APIWrapper>(options1);
aws0.reset(); // good: doesn't call ShutdownAPI
// bad: ShutdownAPI is called at end-of-scope with options1

[OlafvdSpek]

errr. I hear ya. It's not very unusual though. They are turned off in most video game engines (a huge portion of our customer base--which is why we did the whole outcome thing).

How is AWS used in video game engines?

[OlafvdSpek]

Is the mutex necessary? AFAIK you can do an atomic increment, which returns the (previous) value.

[crusader-mike]

Is the mutex necessary? AFAIK you can do an atomic increment, which returns the (previous) value.

you need other threads to wait until initialization completes

[JonathanHenson]

How is AWS used in video game engines?

Shameless plug:

https://www.youtube.com/watch?v=-_2JQYlj3kY
https://www.youtube.com/watch?v=kObVnmtB3nU
https://www.youtube.com/watch?v=7P-9bsx9LdM
https://www.youtube.com/watch?v=UufephokH8A

Also, we have services specifically for games (e.g. GameLift)

then there are other engines such as Unity and Unreal.

[JonathanHenson]

The wrapper doesn't guarantee ShutdownAPI is called with the same options as InitAPI does it?

You're almost right--more right than I am actually. It wouldn't call ShutdownAPI with options1, it would call them with the default constructed Options--which is bad.

But... I can't actually have a Static non-trivial object which std::function prevents. I suppose I'll have to make it a pointer and make that static then do an allocation and a copy after the init call.

Which means..... my options are:

convert SDKOptions to a raw pointer in the c_tor and then I can assert they match. I can also then provide a default ctor that will just use the initial options if they are there. The problem here is that the initial problem space was for a plugin architecture where you wouldn't have access to the original SDKOptions--so that wouldn't work. Also, there may be an overwhelming temptation for users to allocate the options on the heap, which would break the moment I swap out the allocators.

any other ideas?

[crusader-mike]

Options (as I see them):

1. forbid multiple initialization of this lib. It becomes user's headache to deal with other libs that might use this lib
2. get rid of mutable global state (move what you can to AWSClient)
   2a. if there is a 3rd party lib forcing you to have global state -- leave it's initialization to end user (now that lib is his headache, but at least he can init it properly regardless of how this lib is used).

[OlafvdSpek]

it would call them with the default constructed Options

How? I expected options1 to be used.

I suppose I'll have to make it a pointer and make that static then do an allocation and a copy after the init call.

Can't you use a function-level static? No allocation required.

convert SDKOptions to a raw pointer in the c_tor and then I can assert they match.

You can already take the address via the reference.

The problem here is that the initial problem space was for a plugin architecture where you wouldn't have access to the original SDKOptions

Where would you not have access to the original?

I don't know how SDKOptions is used, can the user touch them after InitAIP()?

[diablodale]

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);
    }
}

[OlafvdSpek]

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

[diablodale]

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?

[diablodale]

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

[jmsundar]

@JonathanHenson Is there any update on the implementation of this wrapper? Our project is similar to #456 in that there's a plugin being loaded (possibly multiple times) from a program that is unaware of the SDK. There's no feasible way to share init/shutdown status across instances of the plugin without violating the main program's rules. This would be a very welcome enhancement.

[marcomagdy]

This idea turned out to be too risky and hides bugs related to initialization.

[hguturu]

Looks like this idea proved too risk, but is there an alternative to this approach planned? Or an example for using Aws::InitAPI and Aws::ShutdownAPI in an underlying library, when that library may have functions that may be called in parallel? All the examples, show the init/shutdow calls in a main function? Currently, we have each function calling the init/shutdown, but that makes it so we can't use those functions in threaded code.