Unable to cancel task

Nov 8, 2014 at 7:53 PM
Consider this snippet:
    pplx::task_completion_event<void> ev;
    pplx::cancellation_token_source cts;
    pplx::create_task([ev] 
    {
        Sleep(1000);
        pplx::interruption_point();
        //if (pplx::is_task_cancellation_requested())
        //  pplx::cancel_current_task();
        ev.set();
    }, cts.get_token());
    pplx::create_task([&cts]
    {
        Sleep(500); 
        cts.cancel();
    });
    auto t = pplx::task<void>(ev);
    try
    {
        t.wait();
        std::cout << "Hello World!\n";
    }
    catch (...)
    {
        std::cout << "Error!\n";
    }
I expect it to print "Error!" since the task is cancelled and there is an interruption point in the task that is canceled, but using the latest dev branch, this runs and prints "Hello World!" contrary to my expectations. Am I doing something wrong here? Maybe I'm not understanding how to cancel things properly.

Also, I'm not 100% sure on the semantics of task_completion_event, so this is just some test code. Nevertheless, I've verified using the debugger that evne if cts.cancel() is called before pplx::interruption_point, the function does not throw and continues as usual. Also for some strange reason, pplx::is_task_cancellation_requested can not be found in VS14.

I'm using latest dev because I'm using VS14.
Nov 10, 2014 at 3:30 PM
Further investigation shows that the example that Microsoft has at the URL

http://msdn.microsoft.com/en-us/library/hh873170.aspx

does not work in Visual Studio 14 at all! But it works just fine in 2013, which leads me to believe that this is a bug in Microsoft's Visual Studio 14 implementation. Grrr.
Coordinator
Nov 10, 2014 at 6:47 PM
Edited Nov 10, 2014 at 6:49 PM
Hi,

Could you try replacing your instance of .wait() with .get()? If you check the documentation for these two functions, .wait() will not throw an exception if the task was cancelled. .get(), on the other hand, will throw an exception of type task_cancelled. The reasoning for this is that if you're waiting for the task to complete, you might cancel it so that it finishes faster. On the other hand, if you're getting a result out of the task, that clearly can't happen when you've cancelled it.

Note: thus, if this is printing "Error" in VS 2013, that's actually a bug in our 2013 implementation. Could you verify that this same snippet is what you're using in VS 2013?

roschuma
Nov 10, 2014 at 7:04 PM
Well, first of all, the code didn't work like I wanted it to. It printed success if the task was not canceled as expected. If the task was canceled, the application hung. This is true both in 2013 and 14. The difference is that no exception is thrown from pplx::interruption_point in 14 if the task is canceled. This works fine in 2013. I also tested the sample I posted a link to. It works fine in 2013, but continues forever in 14. I tried changing "wait" to "get," but the fundamental problems remains unsolved. The code didn't work as expected in the first place (the task completion event is not actually canceled when the first task is canceled, which was my assumption, which was wrong).

Now, for what my intention actually was (which I have yet to find a good existing solution) is: I just wanted some kind of "delayed" task. The reasoning is something like this (in peudo code; not taking into account variable lifetime);
void foo()
{
    auto t = pplx::create_task(&bar);
    t.wait(); // Wait until t finished or is canceled; furthermore, if canceled, propagate the exception
}

void bar()
{
    http_listener listener(...);
    listener.support(..., [&listener]&foo_listener);
    listener.open();
    // Wait until foo_listener has accepted an incoming connection; or if canceled, propagate the exception
}

void foo_listener(http_listener& listener, http_request request)
{
    try
    {
        listener.close(); // Want only one incoming connection
        // Validate task
        // Signal bar we're done. Note: If an exception is thrown here, bar must be noted that something went wrong or that the task was canceled.
    }
    catch (...)
    {
        request.reply(400, ...);
        listener.open();
    }
}
I just haven't figured out how to do it in a good way. I was thinking of using the setup above, but yeah, didn't work out that well. My current thought is to use a "delayed" task, a task which you can default constructor and wait on which will only become signaled once the a task has been associated with it and run to completion (or canceled).
Coordinator
Nov 10, 2014 at 8:38 PM
Ok,

The reason the task is hanging forever is because you're waiting on a task constructed from the event ev. While you have a "success" path which will set ev as completed, when your code takes the "failure" path, ev is simply left unset. By cancelling cts you did cancel the task that would normally set ev, but you didn't actually cancel ev itself. In the simple example at the top, it would be better to simply wait on the task itself, but that's tangential to your main use case.

Unfortunately, you need to be prepared to accept multiple simultaneous async callbacks onto foo_listener (in the general case). If you have extra knowledge that this will never happen, it would be easier to not worry about enforcing it at all and simply assume that foo_listener will only be called once.

I'm going to go ahead on the assumption of a single call to foo_listener:
void sync_single_connection() {
    http_listener listener(...);
    task_completion_event<void> ev;
    listener.support([&](...)
    {
        create_task([&](...)
        { /* things go here, even stuff that throws */
        }).then([&](task<void> t)
        {
            try { t.get(); ev.set(); }
            catch (...) { ev.set_exception(std::current_exception()); }
        });
    }); // end of support(...)

    task<void>(ev).get(); // This will throw any exception that happens inside support or block until it completes
}
However, it would be best if you simply wrapped everything you wanted to do inside the task created inside .support(). I don't know the full details of your application, but I think your design may be too "synchronously" focused. In general, try to never call .get() or .wait() except inside a task-based continuation (basically the lambdas that accept pplx::task<T>).

That said, we do use the pattern you're describing here quite often in our tests; please do take a look at those for many more examples of how this can be done.

roschuma
Nov 10, 2014 at 8:53 PM
So here is a more "overview" of what I'm doing and the requirements and why I'm forcing it to be synchronous:

First there is the class X. At construction of X, it creates an async task that establishes a connection to a server and exchanges data back and forth for a while until it's happy. Network always implies latency, and blocking the constructor is always a bad idea. Furthermore, the caller should be free to do whatever else setup may be necessary before using X.

So basically all members in X will block until the async task is complete (and the results from it acquired), because these functions are synchronous, because the code expects it to be.

Could the caller code be asynchronous too? Probably. Actually, it sounds like a pretty neat idea. But I want to focus on getting the code compiling and working against first. Small steps. One at a time.

So the async task can do whatever it wants. I try to keep it as asyncronous as possible with task continuation. That's fine--it can do whatever it wants, as long as it blocks until it's finished, so that X can wait on the async task it spawned.

While I don't doubt the code you wrote works, it's too much boilerplate and too much complexity. Easy to forget and a lot to write each time. I thought of using an "event" (such as pplx::event) to signal when the task is done, but again, if an exception is thrown, that event never gets signaled. That violates the RAII principle--the code should just work even if an exception is thrown without any extra code which your example clearly violates.
(I should make a wrapper akin to BOOST_SCOPE_EXIT to re-open the listener if an exception is thrown.)

Consequently, I am expecting only one call to foo_listener. Multiple calls to foo_listener is an error. So when I have my callback, I try to close it to keep further connections from coming. It's probably not very robust, though (I suppose I'd need a lock for true thread safety here).

So I want to take some time to create proper abstraction, even if I have to write my own. Code quality is a higher concern to me than "something that works." This is just a hobby project, after all.

I'll take a look at the examples, though! Thanks!
Coordinator
Nov 10, 2014 at 8:58 PM
Why not make a task to construct X? Then the caller must call ".get" in order to get the X, which makes the fact that it blocks explicit. The task will run in the background, so the caller can do whatever it likes in the foreground until it decides to block on the X finishing up. At that point, you'll have guaranteed that X is done blocking, so none of its methods need to be synch-blockers anymore.
Nov 10, 2014 at 9:07 PM
Edited Nov 10, 2014 at 9:07 PM
Not bad idea, I'll admit. If I do two-stage construction in X (i.e. a default constructor, another constructor that starts the process and a function that starts the process), it could work. I'll have to test that. Default constructors that block are still evil, though, as it blocks certain things like std::async (learned that the hard way).

Thanks again!