Skip to content
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.

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

Replaced POSIX threads under Windows with stdlib implementations since it is under LGPL license #100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

holoeye-photonics
Copy link
Contributor

The oxygine engine uses POSIX threads under Windows which is a handy library but sadly under LGPL so we cannot link it statically. Since C++ 11 there are official implementations std::mutex, std::thread and std::conditional_variable. So I put those in which gets rid of POSIX threads under Windows which resolves our licensing issue. There is a define OX_CPP11THREADS that needs to be set in order to enable the use of the stdlib classes.

…library but sadly under LGPL so we cannot link it statically. Since C++ 11 there are official implementations std::mutex, std::thread and std::conditional_variable. So I put those in which gets rid of POSIX threads under Windows which resolves our licensing issue. There is a define OX_CPP11THREADS that needs to be set in order to enable the use of the stdlib classes.
@holoeye-photonics
Copy link
Contributor Author

I had to split the mutex class into Mutex and MutexRecursive since the recursive mutex in stdlib is its own type.
I tried a templated autolock class but it did not work nicely so I ended up having two autolock classes, one for Mutex and one for MutexRecursive.

@holoeye-photonics
Copy link
Contributor Author

I noticed that. The ThreadDispatcher is probably still a bit fishy.

When you look at the file you see in line 156, that I put in a todo.

The problem is that POSIX allows you to have a condition for a recursive mutex. The stdlib does not support this. There is a type conditional_variable_any that would take a recursive mutex, but the documentation explicitly says that this will not work.

So I would see two solutions here. Either use two mutex, one for locking one only to wait for the condition, or we use like an atomic int to establish a communication.

My current workaround looks like this which isprobably not safe.
MutexPthreadLock::MutexPthreadLock(std::unique_lock<std::mutex>& l, bool lock) : _lock(l), _locked(false) 27 { 28 if(lock && !_lock.owns_lock()) 29 { 30 _lock.lock(); 31 _locked = true; 32 } 33 }

Do you have a unit test or something that could be used to very if no problem exists? Even just some instructions on how to test this would be fine.
As I said we need the stdlib threads due to licensing issues so I am happy to look at this more and to actually resolve any remaining issues with ThreadDispatcher.

Cheers,
Dan

@holoeye-photonics
Copy link
Contributor Author

I went with the following solution in the end:

` std::recursive_mutex _mutex;

    class
    {
    private:
        std::atomic_uint _in, _out;

    public:
        void wait(std::recursive_mutex &mutex)
        {
            mutex.unlock();

            const unsigned int ticket = ++_in;

            while(_out < ticket)
                std::this_thread::sleep_for(std::chrono::milliseconds(10));

            mutex.lock();
        }

        void notify_all()
        {
            _out = _in;
        }

        void notify_one()
        {
            _out++;
        }
    } _cond;`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant