Boost C++ Libraries: Ticket #12519: boost::thread::try_join_for does not return after timeout

viboes — Fri, 04 Nov 2016 00:11:35 GMT

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

owner changed

viboes — Fri, 04 Nov 2016 00:12:02 GMT

owner changed from Anthony Williams to viboes

viboes — Fri, 30 Dec 2016 02:03:37 GMT

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

viboes — Fri, 30 Dec 2016 02:30:39 GMT

Replying to viboes:

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

Forget this comment, please.

viboes — Sun, 19 Feb 2017 10:52:58 GMT

Please, could you check if

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

could solve the issue?

viboes — Sun, 19 Feb 2017 10:55:10 GMT

Please, could you check if

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

could solve the issue?

milestone changed

viboes — Sun, 19 Feb 2017 10:58:58 GMT

milestone To Be Determined → Boost 1.64.0

status changed

viboes — Sun, 19 Feb 2017 11:01:25 GMT

status new → assigned

lumosimann@… — Wed, 22 Feb 2017 14:17:56 GMT

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@… — Thu, 23 Feb 2017 08:16:20 GMT

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 — Fri, 24 Feb 2017 14:29:30 GMT

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

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

type changed

viboes — Fri, 24 Feb 2017 14:37:47 GMT

type Bugs → Patches

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 — Fri, 24 Feb 2017 15:19:23 GMT

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 — Fri, 24 Feb 2017 15:34:37 GMT

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 — Sat, 25 Feb 2017 21:50:37 GMT

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@… — Mon, 27 Feb 2017 07:18:44 GMT

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@… — Fri, 21 Apr 2017 10:23:53 GMT

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@… — Wed, 17 May 2017 11:52:31 GMT

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@… — Wed, 17 May 2017 17:30:28 GMT

@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 — Sun, 04 Jun 2017 22:52:25 GMT

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@… — Tue, 06 Jun 2017 05:59:58 GMT

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

Fix what I posted in comment #16
Remove BOOST_THREAD_USEFIXES_TIMESPEC and make it default
Rename my_clock_t into something more descriptive and move into detail namespace
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 — Sat, 10 Jun 2017 09:08:37 GMT

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

viboes — Sat, 10 Jun 2017 09:30:34 GMT

I would

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

I have no time now to check for windows :(

viboes — Mon, 12 Jun 2017 19:23:23 GMT

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

lumosimann@… — Tue, 13 Jun 2017 10:36:05 GMT

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.

milestone changed

viboes — Sat, 24 Jun 2017 14:35:49 GMT

milestone Boost 1.64.0 → Boost 1.65.0

status changed; resolution set

viboes — Sat, 24 Jun 2017 14:56:45 GMT

status assigned → closed
resolution → fixed