6
\$\begingroup\$

I don't know C++ at all, but for the heck of it I tossed this together because I was talking with someone about C++ and was given the impression there is absolutely nothing quite like the windows APIs ManualReset and AutoReset EventWaitHandles, so I figured I'd see if I could create one.

I think this might work - it compiles and runs but I don't really know well enough how to make a thread to test it's multi-threading off hand in C++ and don't care to try figuring it out. Figure folks here can probably tell me if I got any of the threading primitives usage completely off or have some harrible race conditions or some such.

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

using namespace std;

class EventWaitHandle {

public:
    EventWaitHandle(bool handleWillCloseAfterOneWait, bool openAtStart)
        : autoReset(handleWillCloseAfterOneWait), handleOpen(openAtStart)
    { }

    void set() {
        {
            std::lock_guard<std::mutex> handleOpenLock(handleOpenLocker);
            handleOpen = true;
        }

        handleOpenSignal.notify_one();
    }

    void reset() {
        {
            std::lock_guard<std::mutex> handleOpenLock(handleOpenLocker);
            handleOpen = false;
        }

        handleOpenSignal.notify_one();
    }

    bool wait(int millisecondsBeforeTimeout) {
        {
            std::lock_guard<std::mutex> handleOpenLock(handleOpenLocker);

            if (handleOpen) {
                handleOpen = !autoReset;
                return true;
            }

            if (millisecondsBeforeTimeout < 1) // Handle isn't open and we are waiting no time, punt..
                return false;

        }

        auto start = std::chrono::system_clock::now();
        cout << "waiting.." << endl;
        std::unique_lock<std::mutex> lockJustForThisWaiter(handleOpenLocker);

        if(handleOpenSignal.wait_until(lockJustForThisWaiter, start + std::chrono::milliseconds(millisecondsBeforeTimeout)) == std::cv_status::timeout)
            return false;

        auto timeLeftToWaitInEventOfSpuriousness = std::chrono::milliseconds(millisecondsBeforeTimeout) - (std::chrono::system_clock::now() - start);
        int millisecondsLeftToWait = 0;
        millisecondsLeftToWait = std::chrono::duration_cast<std::chrono::milliseconds>(timeLeftToWaitInEventOfSpuriousness).count();

        return wait(millisecondsLeftToWait);
    }

private:

    bool autoReset;

    std::condition_variable handleOpenSignal;
    std::atomic<bool> handleOpen;
    std::mutex handleOpenLocker;
};

That's what I want review of, this is just example usage concept:

int main(){
    EventWaitHandle waitHandle(true, true);

    bool open;

    open = waitHandle.wait(100);
    if (open)
        cout << "open" << endl;
    else
        cout << "not open" << endl;

    waitHandle.set();

    open = waitHandle.wait(100);
    if (open)
        cout << "open" << endl;
    else
        cout << "not open" << endl;

    waitHandle.reset();

    open = waitHandle.wait(100);
    if (open)
        cout << "open" << endl;
    else
        cout << "not open" << endl;

    return 0;
}
\$\endgroup\$
1
\$\begingroup\$

Despite the good odds of a tail-call optimisation, your use of recursion seems unidiomatic and sets off alarm bells in my head. Though they are largely unfounded in a practical sense, I'd say that's enough of a warning to just perform loop-waiting with an actual loop, instead.

You could do with some documenting comments, as it's not clear to a non-WinAPI person what this class actually does.

Some of your code lines are too long to read comfortably.

I can't spot any actual bugs though.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.