boost::thread::try_join_for does not return after timeout

[mweb@…]

If we use BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC try_join_for does not abort after the given timeout anymore. If I remove the define it works again.

We use the define to be able to use sleep_for while changing the system time. (See #6787)

To Reproduce:

#define BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC

#include <iostream>
#include <boost/thread.hpp>

void test_func()
{
    boost::this_thread::sleep_for(boost::chrono::milliseconds(1500));
}

int main()
{
    boost::thread t(test_func);

    if (!t.try_join_for(boost::chrono::milliseconds(50))) {
        std::cout << "OK" << std::endl;
    } else {
        std::cout << "FAILED" << std::endl;
    }

    if (t.try_join_for(boost::chrono::milliseconds(2000))) {
        std::cout << "OK" << std::endl;
    } else {
        std::cout << "FAILED" << std::endl;
    }
}

If the define is available the first try_join_for waits till the thread is finished, the expected behavior would be that the first try_join_for would return before the thread is finished.

[viboes]

It seems that the reference ticket doesn't works yet.

[viboes]

Just to be fixed, have you tried defining BOOST_THREAD_VERSION 4?

[viboes]

Replying to viboes:

Just to be fixed, have you tried defining BOOST_THREAD_VERSION 4?

Forget this comment, please.

[viboes]

Please, could you check if

​https://github.com/boostorg/thread/commit/544eda51bdf0c62b2f29dea6fe1cf935ad62580e

could solve the issue?

[viboes]

Please, could you check if

​https://github.com/boostorg/thread/commit/544eda51bdf0c62b2f29dea6fe1cf935ad62580e

could solve the issue?

[lumosimann@…]

We now found a temporary fix for this problem. Please check

​https://gist.github.com/lukasm91/9732228f33e92963bb8762dac519e79f

Note that there are two bugs involved, one in sleep_for, and one in try_join_for. We found two functions where a differentiation between monotonic and non-monotonic time was missing, such that some functions always used monotonic, and others always used non-monotonic time.

There is also an updated test file attached. Consider that when you undefine BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC, this currently needs to be done in libs/thread/src/pthread/thread.cpp too, otherwise the compiler won't find the correct template specialization. The old behavior can be reactivated by undefining USEFIXES.

@viboes Could you please check this fix and incorperate it into boost? We know that most probably a few #ifdefs are missing, but this fix currently solves our problems.

[lumosimann@…]

I have just added a set of google tests to the gist mentioned above (googletests.cpp).

Note that I am not sure whether this is all the expected behavior. Especially, I am unsure about what the expected behavior is when I set the time backwards while a sleep_for is running and BOOST_THREAD_HAS... is undefined.

[viboes]

I've rolled back the last change as it seems it introduce a regression.

​https://github.com/boostorg/thread/commit/9bbf9bed80836282263ec0bdea0adf5c1fa3621b

[viboes]

Thanks a lot for the analysis. Yes, I missed this, and mixed monotonic and non-monotonic clocks :(

I'll try to merge your changes as soon as possible.

[viboes]

I have some trouble with ​https://gist.github.com/lukasm91/9732228f33e92963bb8762dac519e79f#file-boost_12510-patch-L53

-#ifdef BOOST_THREAD_USES_CHRONO
 #include <boost/chrono/system_clocks.hpp>
 #include <boost/chrono/ceil.hpp>
-#endif

Why do you need to remove these lines?

[viboes]

Replying to viboes:

I have some trouble with ​https://gist.github.com/lukasm91/9732228f33e92963bb8762dac519e79f#file-boost_12510-patch-L53

-#ifdef BOOST_THREAD_USES_CHRONO
 #include <boost/chrono/system_clocks.hpp>
 #include <boost/chrono/ceil.hpp>
-#endif

Oh, I see that it wouldn't compile if not Chrono is not there :(

I would need to change this because unfortunately Boost.Thread can be used without Boost.Chrono.

Why do you need to remove these lines?

[viboes]

I've committed

​https://github.com/boostorg/thread/commit/c7348b29cf8bfa1272645d04784419d37e1e7db5

Please, could you tell me if fixes the issue.

You will need to compile Boost.Thread after uncommenting in the config.hpp file or compiling everything with -D

//#define BOOST_THREAD_USEFIXES_TIMESPEC
//#define BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC

As the commit says, this is a partial solution as it needs to define some compiler defines, but at least it could be used to check if the solution works.

I order to don't need the flags I would need to be able to detect BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC and move inline, everything that could depend on it.

I believe that I can remove BOOST_THREAD_USEFIXES_TIMESPEC, but is a little bit too late for boost 1.64.

[lumosimann@…]

I now get a segfault due to infinite recursion.

This is consequence of ​https://github.com/boostorg/thread/commit/c7348b29cf8bfa1272645d04784419d37e1e7db5#diff-8a73888d7fa5a2b2b38f85db61376ba4L517

This line is for WIN32, I don't know whether this makes sense there, but for sure you need to add the fix in the linux section too, i.e.

diff --git a/include/boost/thread/detail/thread.hpp b/include/boost/thread/detail/thread.hpp
index 8dc6a9c..a67e8ea 100644
--- a/include/boost/thread/detail/thread.hpp
+++ b/include/boost/thread/detail/thread.hpp
@@ -542,7 +542,7 @@ namespace boost
         }
 #endif
 #ifdef BOOST_THREAD_USES_CHRONO
-        bool try_join_until(const chrono::time_point<chrono::system_clock, chrono::nanoseconds>& tp)
+        bool try_join_until(const chrono::time_point<my_clock_t, chrono::nanoseconds>& tp)
         {
           using namespace chrono;
           nanoseconds d = tp.time_since_epoch();

Otherwise we will infinitely recurse into try_join_until.

Although this fix is only temporary, I would recommend renaming my_clock_t and move it to the detail-namespace.

Could this also be a fix for ​https://svn.boost.org/trac/boost/ticket/6787?

[lumosimann@…]

Will this problem be fixed until Boost 1.65?

We would still get a segfault when adding BOOST_THREAD_USEFIXED_TIMESPEC because the patch has not been applied correctly.

I think the effort is very little because the solution is almost finished except that it needs to be rewritten in "boost-style" (e.g., probably we should not use a typedef my_clock_t).

[anton.indrawan@…]

I did get the recursive call as well and the patch by lumosimann solved the problem. I also vote it to be integrated into the next release (Boost 1.65).

[anton.indrawan@…]

@viboes: I see the following commit:

​https://github.com/boostorg/thread/commit/30dff7f84ac5731fabeb6727f3b947ac12c3508e

I am wondering why you replace timespec_now by timespec_now_realtime. My application hangs and it blocks indefinitely in boost::condition_variable::do_wait_for.

[viboes]

Please, could you tell me if the problem is there yet?

There were a lot of commits since then, in particular

​https://github.com/boostorg/thread/commit/bf4b38b0af892ac6e5e1f2daaf8f21b80da4d311

[lumosimann@…]

Yes, the problem is still around, because the patch has not been applied correctly. Your commits are unrelated to this.

The patch I submitted in post #comment:9 still works for me. You have applied this patch, but as I have already mentioned in comment #comment:16, you applied the patch at the wrong place. I have provided another patch in that comment which works at least for me.

I would recommend to

1. Fix what I posted in comment #16
2. Remove BOOST_THREAD_USEFIXES_TIMESPEC and make it default
3. Rename my_clock_t into something more descriptive and move into detail namespace
4. Test whether and how this fix is also needed for windows (this I really don't know)

As I mentionned in #comment:9, there are also unittests in the github-gist given in that comment (I would post it again, but the bug tracking system does not allow me to post that link to github).

When you run those tests, you will see that BOOST_THREAD_USEFIXES_TIMESPEC + BOOST_THREAD_CONDATTR_SET_CLOCK_MONOTONIC currently results in a segfault, and all other combinations result in at least one error.

The fix in comment #comment:16 plus defining BOOST_THREAD_USEFIXES_TIMESPEC resolves all unittests with and without BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC set accordingly.

Doing this would solve both issues #12519 and #6787. Therefore I would strongly vote to integrate this into the upcoming boost version.

[viboes]

I will take a look again to the patches. Please, could you provide a github PR?

[viboes]

I would

1. take in account what you posted in comment #16 (I forget it surely)
2. Define BOOST_THREAD_USEFIXES_TIMESPEC for the time being
3. Rename my_clock_t into detail::internal_clock_t

I have no time now to check for windows :(

[viboes]

​https://github.com/boostorg/thread/commit/d4cff01c724f1355fef0990f3d6d678e52f61fd0

[lumosimann@…]

I had a look at your commit. Seems fine to me and works with my tests.

You could post this to #6787 as well, which is likely to be fixed by now.