11

Possible Duplicate:
Is there a way to cancel/detach a future in C++11?

There is a member function which runs asynchronously using std::future and std::async. In some case, I need to cancel it. (The function loads near objects consecutively and sometimes an objects gets out of range while loading it.) I already read the answers to this question addressing the same issue, but I cannot get it work.

This is simplified code with the same structure as my actual program has. Calling Start() and Kill() while the asynchronous is running, causes a crash because of access violation for input.

In my eyes the code should work as follows. When Kill() is called, the running flag is disabled. The next command get() should wait for thread to end, which it does soon since it checks the running flag. After the thread is canceled, the input pointer is deleted.

#include <vector>
#include <future>
using namespace std;

class Class
{
    future<void> task;
    bool running;
    int *input;
    vector<int> output;

    void Function()
    {
        for(int i = 0; i < *input; ++i)
        {
            if(!running) return;
            output.push_back(i);
        }
    }

    void Start()
    {
        input = new int(42534);
        running = true;
        task = async(launch::async, &Class::Function, this);
    }

    void Kill()
    {
        running = false;
        task.get();
        delete input;
    }
};

It seems like the thread doesn't notice toggling the running flag to false. What is my mistake?

Community
  • 1
  • 1
danijar
  • 32,406
  • 45
  • 166
  • 297
  • 7
    Maybe try a `std::atomic running;`? – aschepler Jan 13 '13 at 14:16
  • 1
    This doesn't compile, Function.get()? – ronag Jan 13 '13 at 14:16
  • 2
    Why is `input` a pointer to heap-allocated memory? – Konrad Rudolph Jan 13 '13 at 14:17
  • 1
    first of all, `running` is not synchronized. try declaring it as `atomic`. you may also try using `volatile` and synchronize manually if you wish, most likely the two threads run on different cores and the compiler stores `running` in a registry – Andy Prowl Jan 13 '13 at 14:17
  • You should dereference `input` in your for loop: `for(int i = 0; i < *input; ++i)` – Olaf Dietsche Jan 13 '13 at 14:22
  • 1
    @AndyProwl. Thanks, since I am new to threading I never heard of them so I will do some research. I fixed the typos referred by ronag and Olaf Dietsche. – danijar Jan 13 '13 at 14:27
  • @KonradRudolph. Because this is simplified example code. My read program uses a huge three dimensional array as input. – danijar Jan 13 '13 at 14:28
  • @AndyProwl, `volatile` won't help here, it has nothing to do with threads. – Jonathan Wakely Jan 13 '13 at 14:31
  • @JonathanWakely. I am sorry, I had no idea what atomic means when I read the other question. – danijar Jan 13 '13 at 14:39
  • 2
    @JonathanWakely: could you please expand a bit on why it has nothing to do with threads? `volatile` forces the compiler not to store that variable in a registry, so that changes made by a thread running one core are forced to be seen by threads running on other cores. of course this doesn't mean there is no data race, which is why i pointed out `atomic` should be used – Andy Prowl Jan 13 '13 at 14:39
  • @AndyProwl. I wish I could accept your first comment as answer, everything is fine using `atomic_bool` or `atomic`. I you would write an answer, I will accept that. – danijar Jan 13 '13 at 14:44
  • sharethis: no need for that. also because @aschepler preceded me by a few seconds while i was writing my comment, so he could get credit as well. – Andy Prowl Jan 13 '13 at 14:46
  • I see, thanks to @aschepler, too! I am so happy because I spend the last several hours on this threading part. – danijar Jan 13 '13 at 14:51
  • 2
    @AndyProwl `volatile` only prevents the compiler from optimizing away memory accesses to the variable but doesn't prevent the CPU from reordering memory accesses, thus the changes to memory made by one thread my appear to have happened in different order to another thread. This is the reason for existence of atomics and locks which employ appropriate memory barriers to ensure that changes are visible to other threads in correct order. When using atomics or locks `volatile` is unnecessary. – Maxim Egorushkin Jan 13 '13 at 15:22
  • @AndyProwl, NO NO NO! `volatile` only has affects within a single thread by preventing compiler reordering of the code, it does **nothing** to prevent hardware reordering and does nothing to synchronise memory between threads. C++'s volatile is not like Java's volatile. See http://www.hpl.hp.com/personal/Hans_Boehm/c++mm/old_snapshots/mm042905.html#volatile and http://www.drdobbs.com/parallel/volatile-vs-volatile/212701484 and write it out 100 times: volatile is useless for thread safety – Jonathan Wakely Jan 13 '13 at 15:23
  • 1
    @MaximYegorushkin thank you, Max, you beat me to it – Jonathan Wakely Jan 13 '13 at 15:24
  • 7
    @JonathanWakely it must be the lack of proper synchronization ) – Maxim Egorushkin Jan 13 '13 at 15:26
  • 1
    To be a bit more explicit, if using `atomic` then `volatile` adds no benefit, the code is already safe against races and visibility problems. If not using `atomic` (or non-standard equivalents) then `volatile` is not sufficient for correctness, so you should be using `atomic`. `volatile` is for accessing hardware, not for multithreading. – Jonathan Wakely Jan 13 '13 at 15:30
  • @JonathanWakely: first of all, no need to shout. secondly, I never stated `volatile` in C++ helps preventing data races. thirdly, i never mentioned Java's `volatile`, which is indeed something completely different (btw, I am not a Java programmer). i never mentioned `volatile` does replace synchronization in any possible way. – Andy Prowl Jan 13 '13 at 15:34
  • @MaximYegorushkin: as I wrote, I am not asserting that using `volatile` itself prevents data races. `volatile` just forces the compiler not to store that variable on a register. of course `atomic` is the first option here, which is why I wrote it first btw – Andy Prowl Jan 13 '13 at 15:36
  • 2
    So don't even mention volatile in this context, it's useless and irrelevant to the question. All it does is propagate the incorrect belief that `volatile` is in any way relevant to multithreading. – Jonathan Wakely Jan 13 '13 at 15:36
  • @JonathanWakely: I do not think `volatile` is irrelevant to multithreading, which is the topic of this question. for instance, read this: http://www.drdobbs.com/cpp/volatile-the-multithreaded-programmers-b/184403766. sure, `atomic` does solve the whole issue, but mentioning a little bit more about **related** topics does not seem like a crime to me – Andy Prowl Jan 13 '13 at 15:40
  • 2
    @AndyProwl: the idea of Alexandrescu is completely different. He is using `volatile` as a **tag** exactly because it's otherwise useless and thus unused; but it's just that, a **tag**, had he been able to use a `helloworld` tag instead he might have. – Matthieu M. Jan 13 '13 at 16:02
  • @MatthieuM.: I'm sorry, but that's not what the "Back to Primitive Types" section says. I mean it clearly states that declaring the variable as `volatile` is **not enough do prevent data races** (you need mutexes for that), but it also states that **it must be done** (*"If Increment and Decrement are to be called from different threads, the fragment above is buggy. First, ctr_ **must** be volatile"*). Now if Alexandrescu is wrong as JonathanWakely says, then I'm sorry, I always found him a reliable source. I know that C++11 has a better primitive. I disagree `volatile` is unrelated. Stop. – Andy Prowl Jan 13 '13 at 16:08
  • @MatthieuM. More on the subject of _volatile tag_ for thread safe member functions: Herb Sutter postulates that in C++11 `const == thread safe`, see http://channel9.msdn.com/posts/C-and-Beyond-2012-Herb-Sutter-You-dont-know-blank-and-blank – Maxim Egorushkin Jan 13 '13 at 16:10
  • 2
    @MaximYegorushkin: Herb Sutter is extremely imprecise in that statement. First, what he actually means is "const => thread safe" and not "==" (he updated his slides). Second, even that is an oversimplification: "const" simply implies "introduces no data races with other read operations", where "data race" is defined per paragraph 1.10. This is what Sutter actually wants to say. – Andy Prowl Jan 13 '13 at 16:12

1 Answers1

11

Since noone's actually answered the question yet I'll do so.

The writes and reads to the running variable are not atomic operations, so there is nothing in the code that causes any synchronisation between the two threads, so nothing ever ensures that the async thread sees that the variable has changed.

One possible way that can happen is that the compiler analyzes the code of Function, determines that there are never any writes to the variable in that thread, and as it's not an atomic object writes by other threads are not required to be visible, so it's entirely legal to rearrange the code to this:

void Function()
{
    if(!running) return;
    for(int i = 0; i < *input; ++i)
    {
        output.push_back(i);
    }
}

Obviously in this code if running changes after the function has started it won't cause the loop to stop.

There are two ways the C++ standard allows you to synchronize the two threads, which is either to use a mutex and only read or write the running variable while the mutex is locked, or to make the variable an atomic variable. In your case, changing running from bool to atomic<bool> will ensure that writes to the variable are synchronized with reads from it, and the async thread will terminate.

Jonathan Wakely
  • 166,810
  • 27
  • 341
  • 521
  • From all the comments, I agree that `volatile` is not enough. But can you explain what happens if `running` was declared `volatile` but not atomic. I guess such re-ordering of running before the for loop Is not allowed, so what else could go wrong in this example? – balki Jan 14 '13 at 17:52
  • The compiler wouldn't reorder it, but without some memory barriers (which `volatile` does not imply) there's no guarantee that a write to a volatile variable would be visible to a thread running on a different CPU. – Jonathan Wakely Jan 14 '13 at 18:07
  • Is it not guaranteed to be visible immediately on a different CPU or could it happen that the write to the `volatile` variable was never visible to other CPU at all!? In the above case, even if `running` was not visible for a couple of more iteration, it is not harmful. – balki Jan 15 '13 at 10:06
  • 1
    There is no guarantee it will ever become visible before the end of the universe. Do not use `volatile` for inter-thread synchonization. Use the features designed for inter-thread synchronization which give useful guarantees. Your question is about C++11, so use the appropriate C++11 features. – Jonathan Wakely Jan 15 '13 at 10:57