zmq::condition_variable_t needs cleanup

[sigiesec]

zmq::condition_variable_t needs some cleanup:

• the implementation under _WIN32_WINNT < 0x0600 && !_SUPPORT_CONDITION_VARIABLE is unusable, it asserts in the constructor. since this is detected at compile-time, all depending functionality (i.e. thread-safe sockets), should also be disabled at compile-time
• the implementation under _SUPPORT_CONDITION_VARIABLE && (defined(ZMQ_HAVE_WINDOWS_TARGET_XP) || _WIN32_WINNT < 0x0600) uses a second mutex for syntactic reasons: since zmq::condition_variable_t::wait accepts a zmq::mutex_t, but std::condition_variable::wait requires a std::unique_lockstd::mutex, this overhead is required. This could be resolved by
  • using this implementation iff mutex_t (or maybe a specific cv_mutex_t, if std:.mutex should not be used in other cases) is std::mutex
  • define zmq::scoped_lock_t as std::unique_lock<mutex_t> in this case
  • change the parameter of zmq::condition_variable_t::wait from zmq::mutex_t to zmq::scoped_lock_t

As pointed out in #3373 (comment), the current implementation in the second case seems to have a race condition related to handling the two mutexes. This would be automatically resolved by the improvements suggested above.

[somdoron]

Agree, if std:: conditional_variable is used we should use all std primitives for synchronization.
The current implementation is at least slow and worst doesn't work.

[sigiesec]

@bluca This is not Windows-specific, it applies to all C++11-enabled platforms.

[bluca]

An sorry, got tricked by the ifdef

[WallStProg]

This is the bit that makes me suspicious:

inline int wait (mutex_t *mutex_, int timeout_)
{
    std::unique_lock<std::mutex> lck (_mtx); // lock mtx
    mutex_->unlock ();                       // unlock mutex_
    ...
    lck.unlock ();   // unlock mtx
    *** RACE CONDITION HERE? ***
    mutex_->lock (); // lock mutex_
    return res;
}

What appears to be happening is that the method is using a local variable for the condition variable, but the order of lock/unlocks looks wrong to me. I would generally expect to see the lock/unlocks nested, like so, if the purpose of the local (outer) mutex is to protect the inner mutex:

1. outer lock
2. inner unlock
3. inner lock
4. outer unlock

Instead, the locks "cross" each other, so there is a possibility that the inner mutex can be locked by another thread between step 3 and step 4:

1. outer lock
2. inner unlock
3. outer unlock
4. inner lock

Maybe this is deliberate? But again it made me suspicious because it doesn't fit the usual pattern.

Frankly, it's also not at all clear to me what the code intends, and apparently I'm not the only one who finds it confusing.

P.S. If the locks/unlocks are re-ordered, there would be no need to explicitly unlock the outer mutex -- RAII would take care of it.

[WallStProg]

An sorry, got tricked by the ifdef

At least I'm not the only one ;-)

Just a thought -- but at some point would it be preferable to break out the implementation into separate header files (e.g., condition_variable_vxworks.hpp etc.) to make it more readable?

[sigiesec]

Yes, the ifdef is confusing, I think I mistook it somehow as well, either what I wrote in the issue is not correct, or my later assumption is not. But I don't see a reason why the C++11 construct should only be used on Windows. The conditions should be cleaned up to be less confusing as well.

With respect to splitting into multiple files: I am not sure if this should be done, it has benefits but also drawbacks (e.g. when changes in the API are made, it is harder to see the different places where changes must be done, for other platforms that the person doing the change has not in focus). But if it is done, it should be done consistently throughout libzmq IMO.

[WallStProg]

A couple of points:

• The choice of whether to use C++1x features should not be based on the compiler's level of support. In our case, we deliberately disable the C++11 support, and force libzmq to build with --std=gnu++89 to allow us to build a single executable that runs on all our servers.
  Anything that can be set outside the CMakeLists.txt should be, so for instance in our case:

# do the build
$(which cmake) \
   -DCMAKE_CXX_FLAGS="-fno-omit-frame-pointer -std=gnu++98 -D_GLIBCXX_USE_CXX11_ABI=0" \
   -DCMAKE_C_FLAGS="-fno-omit-frame-pointer -std=gnu99" \
   ...

I'm happy to submit a PR for this, although it's simply a matter of deleting the existing checks.

• As far as splitting files go, one benefit of that approach is it works better with IDE's, most of which seem to get hopelessly confused with the tricksy #ifdef's (as do I ;-).

[bluca]

iirc if you force the compiler to use a version of the standar, then all the definitions will respect that

[WallStProg]

iirc if you force the compiler to use a version of the standar, then all the definitions will respect that

That's true, but what I'm referring to is the way the standard is set:

include(CheckCXXCompilerFlag)
check_cxx_compiler_flag("-std=gnu++11" COMPILER_SUPPORTS_CXX11)
if(COMPILER_SUPPORTS_CXX11)
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=gnu++11")
endif()

[bluca]

right, try to pass the CXXFLAGS manually and see if cmake appends or prefixes - if the former, then it should just work as the last argument wins

[somdoron]

One thing that will actually make the conditional variable code easier is to merge it with mutex code.
That the reason for the weird implementation of c++11 conditional variable.

So we can merge the code of the conditional variable into the mutex class. Create both of them on the constructor and providing the conditional variable method on the mutex as well.

[sigiesec]

I think if there are multiple implementations that can be used in an environment, the choice should be made during configuration (e.g. CMake call, configure call), not "automatically" during compilation. A reasonable default should be chosen (e.g. use the C++11 implementation if available), but this should be overridable by the user. This does not only apply to condition_variable_t, but also to several other aspects. If this is done systematically, I think the ifdef conditions will get much simple throughout the source code. I have had this thought for quite some time, but did not find time to do this. But for now, I can start with this particular issue.

[somdoron]

In C# Mutex and Conditional Variable is actually the same class, I think we can benefit from that as well in libzmq.

https://docs.microsoft.com/en-us/dotnet/api/system.threading.monitor?view=netframework-4.7.2

[WallStProg]

Quite agree with @sigiesec on this.

A good model is ENABLE_DRAFTS -- this is defaulted based on the presence or absence of a .git directory, but can be overridden on command line.

While Luca's suggestion likely works (haven't tried it though), it's a bit "opaque" and Simon's approach is, I think, cleaner.

[bluca]

Having flags is fine, I was just suggesting a way to try it immediately without code changes