New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`cancel` is not synchronous #41

Closed
bitonic opened this Issue Jul 8, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@bitonic
Contributor

bitonic commented Jul 8, 2016

cancel returning does not mean that the canceled Async has indeed been terminated, and this resulted in very surprising behavior in some parts of our codebase. This is due to the fact that there is no guarantee that throwTo has reached the thread (and obviously that the thread has terminated) after it has returned.

This means that when using withAsync, or race, or concurrently if one of the two threads throws an exception, the "late" thread will linger on past their invocation.

Repro by @snoyberg :

#!/usr/bin/env stack
-- stack --resolver lts-6.4 runghc --package async
{-# LANGUAGE OverloadedStrings #-}
import Control.Concurrent
import Control.Exception
import Control.Concurrent.Async
import Control.Monad
import qualified Data.ByteString.Char8 as S8

main :: IO ()
main = do
    race quick infinite >>= (S8.putStr . (`S8.append` "\n") . S8.pack . show)
    S8.putStr "definitely left race\n"
    threadDelay 10000000

quick :: IO ()
quick = do
    S8.putStr "quick\n"
    threadDelay 100000

infinite :: IO ()
infinite =
    (forever $
     do S8.putStr "infinite\n"
        threadDelay 10000) `onException`
    cleanup
  where
    cleanup = do
        threadDelay 2000000
        S8.putStr "still alive!\n"

running this script will result in

infinite
quick
infinite
infinite
infinite
infinite
infinite
infinite
infinite
infinite
infinite
Left ()
definitely left race
still alive!

Many thanks to @kantp for finding out about this behavior.

@bitonic

This comment has been minimized.

Contributor

bitonic commented Jul 8, 2016

Note that race/concurrently do not use withAsync as an optimization, but their implementations match its semantics.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jul 8, 2016

Note that I'm playing with test cases and fixes for this on a branch at https://github.com/fpco/async/tree/children_survive. I'll open a PR when it's ready.

@simonmar

This comment has been minimized.

Owner

simonmar commented Jul 8, 2016

cancel doesn't currently guarantee that the target thread has terminated, but it does guarantee that the exception has been delivered (the docs for cancel are pretty explicit about that).

Do we want the additional guarantee that the thread has terminated? Perhaps...

@bitonic

This comment has been minimized.

Contributor

bitonic commented Jul 8, 2016

Ah, I had misunderstood the semantics of throwTo, I thought it was non blocking but looking at the docs it seems it is: https://www.stackage.org/haddock/lts-6.6/base-4.8.2.0/Control-Concurrent.html#v:throwTo .

I really think cancel should return once the thread has terminated. The pattern that we tripped over in many places is starting many workers, and restarting all of them when one of them fails, e.g.:

forever $ do
  mbExc <- try (race loop1 loop2)
  -- Do something with the exception...

Using async currently the workers cannot be easily restarted while knowing that the old workers are dead.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jul 8, 2016

I'm more ambivalent about cancel, but I definitely think the current
behavior of race and concurrently, and to a lesser extent witgAsync, is
incorrect.

On Fri, Jul 8, 2016, 5:57 PM Francesco Mazzoli notifications@github.com
wrote:

Ah, I had misunderstood the semantics of throwTo, I thought it was non
blocking but looking at the docs it seems it is:
https://www.stackage.org/haddock/lts-6.6/base-4.8.2.0/Control-Concurrent.html#v:throwTo
.

I really think cancel should return once the thread has terminated. The
pattern that we tripped over in many places is starting many workers, and
restarting all of them when one of them fails, e.g.:

forever $ do
mbExc <- try (race loop1 loop2)
-- Do something with the exception...

Using async currently the workers cannot be easily restarted while
knowing that the old workers are dead.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#41 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AADBB7Sye6-sNpmFXPZvJaGJq7-DljDZks5qTmVWgaJpZM4JH6MW
.

@simonmar

This comment has been minimized.

Owner

simonmar commented Jul 11, 2016

I think you're asking for

withAsync action inner = 
  bracket 
    (async action) 
    (\a -> uninterruptibleMask_ (cancel a >> wait a))
    inner

as a specification, right? And then race would wait for the child thread uninterruptibly.

This widens the possibility for uninterruptible deadlock, which worries me a bit.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jul 11, 2016

I'm not sure if we need any masking on that call, if an async exception arrives I'd be fine with the withAsync exiting. Also, I'd replace the cancel a >> wait a with cancel a >> waitCatch a, since I don't care about exceptions coming from the inner action after canceling it.

@bitonic

This comment has been minimized.

Contributor

bitonic commented Jul 11, 2016

Expanding on what @snoyberg says, I think I'm asking for

newCancel x = cancel x <* waitCatch x

so that this behavior is on by default on everything that uses cancel.

I think the best path forward is to have

-- | Synchronous version, waits for the thread to die
cancel :: Async a -> IO ()

-- | Asynchronous version, might return before the thread stopped running
asynchronousCancel :: Async a -> IO ()

@simonmar simonmar closed this Jul 11, 2016

kantp pushed a commit to fpco/libraries that referenced this issue Jul 11, 2016

Philipp Kant
mesosync, async variation with synchronous cancel.
This is a workaround for simonmar/async#41,
which we didn't expect to be closed soo fast.  Still, using this might
be less painful than updating async and its transitive dependencies in
the docker image.

kantp pushed a commit to fpco/libraries that referenced this issue Jul 11, 2016

Philipp Kant
mesosync, async variation with synchronous cancel.
This is a workaround for simonmar/async#41,
which we didn't expect to be closed soo fast.  Still, using this might
be less painful than updating async and its transitive dependencies in
the docker image.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment