-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Problem: pthread condvar timeouts are broken #2967
Conversation
src/condition_variable.hpp
Outdated
@@ -183,7 +183,7 @@ class condition_variable_t | |||
{ | |||
pthread_condattr_t attr; | |||
pthread_condattr_init (&attr); | |||
#ifndef ZMQ_HAVE_OSX | |||
#if !defined(ZMQ_HAVE_OSX) || (defined(ZMQ_HAVE_ANDROID) && !defined(HAVE_PTHREAD_COND_TIMEDWAIT_MONOTONIC)) | |||
pthread_condattr_setclock (&attr, CLOCK_MONOTONIC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still failing the android build job. Maybe the condition is missing some brackets?
A build failure is introduced in the Android build, could you please fix it?
|
@@ -212,8 +219,16 @@ class condition_variable_t | |||
timeout.tv_sec++; | |||
timeout.tv_nsec -= 1000000000; | |||
} | |||
|
|||
#ifdef ZMQ_HAVE_OSX | |||
rc = pthread_cond_timedwait_relative_np ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found pthread_cond_timedwait_relative_np
recommended at least 6 years ago for macOS.
For Android, looks like that was the wrong check, and I should just be checking for r21 or not:
https://stackoverflow.com/questions/44580542/ndk-builder-r15-finds-neither-have-pthread-cond-timedwait-monotonic-nor-pthread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's referenced in https://opensource.apple.com/source/Libc/Libc-166/pthreads.subproj/pthread_cond.c, which is literally OS X 10.0. So yeah I'd say it's probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android build fixed. The clang format build is still erroring, but not on my changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for your contribution, merged
This fixes a race condition / hang for threadsafe sockets. Context: zeromq/czmq#1873 (comment)
Problem: pthread condvar timeouts are broken
Solution:
pthread_cond_timedwait_relative_np()
on macOSpthread_cond_timedwait_monotonic_np()
on old versions of Androidpthread_condattr_setclock(CLOCK_MONOTONIC)
on other platformsContext: zeromq/czmq#1873 (comment)