0

I have a timer class that I have written. Any single timer seems to work perfectly. I was trying to add a load of them to a vector and start a load of them, but because it has a thread member I needed to write a move/copy constructor. I decided to make it move only (non copy). So now I am able to add the timer to a vector (note link to full code at the bottom).

However I noticed some odd behaviour. Here is a how I am adding the timers to a vector (see NOTE 1 and 2):

int main()
{
    // Due to godbolt's limited resources (I think) I could only add 3
    constexpr size_t num = 3;
    std::vector<timer> timers;
    for (size_t i = 0; i < num; i++)
    {
        timer tmr;
        timers.emplace_back(std::move(tmr));
        // NOTE 1: this way does not work quite right
        // timers[i].start(100, [i]{ std::cout << i << std::flush; }, false);
    }

    // NOTE 2: So I now start the timers in a second loop which seems to work
    for (size_t i = 0; i < num; i++)
    {
        timers[i].start(100, [i]{ std::cout << i << std::flush; }, false);
    }

    // Let the timers run for a while
    SLEEP_MSEC(1000);

    std::cout << "\n============ end ==============" << std::endl;
    return 0;    
}

When I run this I get the following output (each timer should print its number, so 0,1 and 2):

move c'tor
~timer()
move c'tor
move c'tor
~timer()
~timer()
move c'tor
move c'tor
move c'tor
~timer()
~timer()
~timer()
210120210102021210120201102   <------- HERE all timers running
============ end ==============
~timer()
~timer()
~timer()

When I remove the second loop and re-enable starting the timer inside the first loop I get (each timer should print its number, but only 2 is printing):

move c'tor
~timer()
move c'tor
move c'tor
~timer()
~timer()
move c'tor
move c'tor
move c'tor
~timer()
~timer()
~timer()
222222222       <-------- HERE only the last timer seems to run
============ end ==============
~timer()
~timer()
~timer()

Here is the full code on godbolt setup for the non-working loop: https://godbolt.org/z/fFtjt2

Both seem to output the correct number of move/destructors and at the right times etc. But I can't spot the issue here. My guess is that I have done the move constructor wrong, but I can't see why.

code_fodder
  • 15,263
  • 17
  • 90
  • 167
  • 2
    Your `timer` class exhibits undefined behavior when it's moved from while started. The lambda running on the thread still holds the pointer to the original object, now moved-from and no longer valid, possibly even destroyed already. It still uses the mutex and condition variable from the old object, not the new one. That's why things work when you start timers after the vector is fully populated, but not when you start while populating (the vector may need to reallocate storage and move its elements around). – Igor Tandetnik May 31 '20 at 20:59
  • @IgorTandetnik thanks, that makes sense. Previously, I did have my code starting the timer before moving it into the vector - that was bad!, but now I have the timer moved before I start it - I had thought that would "fix" it (I don't want the timer to be moved once running - so I probably need a way of dealing with that, but not got that far yet). – code_fodder May 31 '20 at 21:09
  • I don't really want the class to be moveable in general, but I want to be able to create a vector of them (for example).... this is my goal – code_fodder May 31 '20 at 21:10
  • 1
    You can [reserve](https://en.cppreference.com/w/cpp/container/vector/reserve) space using `timers.reserve(num);` before your for loop. That will avoid the reallocation. – 1201ProgramAlarm May 31 '20 at 21:17
  • 1
    You could have a vector of `timer` pointers (possibly smart, e.g. `std::unique_ptr`) to timers allocated on the heap. – Igor Tandetnik May 31 '20 at 21:27
  • @1201ProgramAlarm that certainly works : ) (see my question below) – code_fodder Jun 01 '20 at 05:58
  • @IgorTandetnik I did not try this, but instead did the same thing with a `std::deque` and that also works (see question below) – code_fodder Jun 01 '20 at 05:59
  • I think I have mis-understood something here. It does appear that the vector re-allocation is causing my issue. But I did not understand *why* it is causing an issue. Could you explain that for me? - as I mentioned in a comment, I am starting the timer *after* it has been moved into the vector which I thought would overcome the issue. Thanks : ) – code_fodder Jun 01 '20 at 06:01
  • To combat against moving a running timer, I think I will enforce a stop of the timer when the move c'tor is called (unless there is another way around that?) - but the lambda is embedded inside the start function, so this is tricky. – code_fodder Jun 01 '20 at 06:04
  • Vector manages a contiguous block of memory. When that block is full and you add a new element, the vector needs to allocate a bigger block, then copy or move elements from the old to the new one, then destroy the originals and free the old memory. – Igor Tandetnik Jun 01 '20 at 10:55
  • @IgorTandetnik thank you for the explanation. I think did not ask clearly what I did not understand. I understood that vector can re-allocate, but I am not sure why it reallocated as a result of running the `start(...)` function because the timer has already been added to it by then. The only thing I can think of is the allocation of the lambda/thread but they are (IMHO) already allocated as part of the timer class memebers. Anyway, if you want to put this down as an answer I will mark it up. thanks very much :) – code_fodder Jun 01 '20 at 11:34
  • 1
    It reallocates as a result of running `emplace_back`. It doesn't know nor care about `start` – Igor Tandetnik Jun 01 '20 at 14:28
  • @IgorTandetnik ah, hangon - I think I get it. Its not the current timer that is going wrong its all the previously started timers that have gone wrong! - I was focused on the timer `i` in the loop, but since emplace_back can mean the vector is reallocated, timer `i` is fine, but timers `0` to `i-1` are the ones that can be broken... totally get it now, thanks – code_fodder Jun 01 '20 at 14:50

0 Answers0