1

I'm having trouble understanding conditional_variable wait_for.

Sample program:

#include <iostream>
#include <string>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <chrono>

std::mutex m;
std::condition_variable cv;
bool ready = false;

using namespace std;

void worker_thread()
{
    std::unique_lock<std::mutex> lk(m);

    for (int i=0; i<3; i++) {
      auto remaining = chrono::milliseconds::max();

      std::cout << "Going to sleep for " << remaining.count() << endl;
      bool r = cv.wait_for(lk, remaining, [] () { return ready;});
      std::cout << "Wakey wakey " << r << endl;
    }

    std::cout << "Thread finished" << endl;
}

int main()
{
    std::thread worker(worker_thread);
    worker.join();
}

What I expect this program to do:

  • Go to sleep forever since I never set ready to true nor called cv.notify_one()

What it actually does:

Going to sleep for 9223372036854775807
Wakey wakey 0
Going to sleep for 9223372036854775807
Wakey wakey 0
Going to sleep for 9223372036854775807
Wakey wakey 0
Thread finished

Is this because of "spurious awakenings"? But I used overload (2) which says in the documentation: This overload may be used to ignore spurious awakenings.

Execute code here: https://repl.it/repls/EnchantedFatalUnderstanding

Gillespie
  • 5,780
  • 3
  • 32
  • 54
  • Hint: Try this: `auto remaining = chrono::milliseconds(90000);` – David Schwartz Apr 24 '18 at 23:06
  • @DavidSchwartz What the... it works. Is `chrono::milliseconds::max()` bad (I just wanted it to sleep indefinitely) - some kind of overflow? – Gillespie Apr 24 '18 at 23:10
  • @RPGillespie "*some kind of overflow?*" - sounds like it – Remy Lebeau Apr 24 '18 at 23:21
  • @RemyLebeau So is that a bug in `conditional_variable` I should report to GNU ..? The API says it should be able to take a `chrono::duration` – Gillespie Apr 24 '18 at 23:51
  • @RPGillespie true, but it doesn't say it takes a `max` duration. [cppreference says](http://en.cppreference.com/w/cpp/thread/condition_variable/wait_for) the implementation is equivalent to `std::chrono::steady_clock::now() + rel_time`, so it could be an overflow when adding `milliseconds::max` to `steady_clock::now()`. You should test that separately and see if it works or not before reporting a bug to GNU – Remy Lebeau Apr 24 '18 at 23:57
  • @RemyLebeau Ah... that's exactly what's happening. It's overflowing to a negative value when `std::chrono::steady_clock::now()` is added to it which makes `wait_for` return right away. Maybe just the documentation needs updating. – Gillespie Apr 25 '18 at 01:04
  • VC++ does the same thing. I would not report it. There is no way for the implementation to make max duration depend on a future `now().` The only "fix" would be to remove max duration from the standard. Just use reasonable values. – Jive Dadson Apr 25 '18 at 01:12
  • 1
    You could lift `days`, `weeks`, `months` and `years` [from here](https://github.com/HowardHinnant/date/blob/master/include/date/date.h#L146-L156) and then `wait_for` `years{5}`. Makes the code very readable, and for all practical purposes, does exactly what you want. `std::chrono::years` coming to a standard near you -- in about 2 more years... – Howard Hinnant Apr 25 '18 at 01:32
  • 1
    @HowardHinnant As it turns out, we just so happen to have the `date` library already in our repository - would be trivial to add it to this application. Thanks for the tip. – Gillespie Apr 25 '18 at 01:54
  • @HowardHinnant Hmm. I can see how you can define days and weeks, but how can you define months and years in usable constant units? :-) Perhaps lunar_month and sidereal_year? – Gem Taylor Apr 25 '18 at 11:06
  • Having looked in the libraries previously on linux, there is nothing magical about max(), it is just the underlying storage max, and there is nothing magical in the conversion or adding methods to handle max() - they just overflow. So don't use max! – Gem Taylor Apr 25 '18 at 11:17
  • @GemTaylor [Here is the full documentation](https://howardhinnant.github.io/date/date.html) for the calendar extensions to chrono. The `durations` `years` and `months` have `Period`s which are exactly the average as defined by the civil calendar. These durations can be used both for _chronological_ computations **and** _calendrical_ computations. For example [see this answer](https://stackoverflow.com/a/43018120/576911) for the distinction. – Howard Hinnant Apr 25 '18 at 13:04
  • Fair enough. I don't think I could bring myself to rely on these averages, unless they were clearly labelled as such. I feel it would be all too easy to write alarm(now()+months(6)) without realising that it was an approximation. – Gem Taylor Apr 25 '18 at 13:20
  • @RPGillespie's question here is an excellent example of when a _chronological_ computation (using `years`) is sufficient. There's no need to go to the extra expense of a calendrical computation in this example. – Howard Hinnant Apr 25 '18 at 13:21
  • I don't disagree that there is perhaps a place for it. It just needs properly labelling, otherwise it will be used with inappropriate expectations, and given that the result is not immediate, the bugfixing won't be either. – Gem Taylor Apr 25 '18 at 15:13

0 Answers0