Skip to content
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

Ability to cancel a linked thread without killing self #25

Closed
ozataman opened this issue Jan 10, 2015 · 8 comments
Closed

Ability to cancel a linked thread without killing self #25

ozataman opened this issue Jan 10, 2015 · 8 comments

Comments

@ozataman
Copy link

I wonder if anybody has run into this pattern (and whether I'm missing something in the existing functionality that could already do it):

data InternalCancel = InternalCancel deriving (Eq,Show,Read,Ord,Typeable)
instance Exception InternalCancel

-------------------------------------------------------------------------------
-- | Spawn action and monitor it by linking it to current thread. If
-- we later cancel it with 'cancelLinked', it dies but does not
-- re-raise the exception in our current thread.
asyncLinked :: IO () -> IO (Async ())
asyncLinked f = do
    a <- async (f `catch` (\ InternalCancel -> return ()))
    link a
    return a


-------------------------------------------------------------------------------
-- | Cancel a linked action without killing its supervising thread.
cancelLinked :: Async a -> IO ()
cancelLinked a = cancelWith a InternalCancel
@Yuras
Copy link
Contributor

Yuras commented Oct 23, 2015

@ozataman Could you please describe your use case? (I'm collecting different concurrency patterns)

This code:

bracket (asyncLinked child_code) cancelLinked $ \_ ->
  main_code

seems to be equivalent to the next:

snd <$> concurrently child_code main_code

Does it cover your use case?

@skogsbaer
Copy link

I have a similar problem. That's why I created issue request #64

@simonmar
Copy link
Owner

I actually don't like link very much because it turns synchronous exceptions into asynchronous exceptions, which can cause problems if you assume that children of AsyncException are asynchronous and other exceptions are synchronous, which is the only way we have to distinguish right now.

I'm curious, in your use case for link, is it possible to use wait instead, or somehow refactor to avoid using link?

@ozataman
Copy link
Author

I recently had another use case where 4-5 long running async threads were getting created throughout the application that had to be up and running simultaneously at all times - and if one died, I wanted the whole thing to die and retry from scratch.

After experiencing quite a bit of unreliability (possibly of my own making) from using link ad-hoc in-situ to achieve this, I instinctively ended up re-wiring things to pool all the asyncs at the end and calling something like waitAnyCancel [a1, a2, a3, a4, a5]. It's worked much better so far by a long shot.

I do wonder if my original use case, which was quite a bit more complex, would allow for that as well. May be worth a look at some point.

@ntc2
Copy link
Contributor

ntc2 commented Oct 25, 2017

I wanted to be able to cancel a linked thread, so I came up with

a <- async $ handle (\ThreadKilled -> return ()) $ ...
link a
...
cancel a 

which works because cancel sends ThreadKilled.

However, according to the ThreadKilled docs, there are other (mysterious) ways for ThreadKilled to be raised:

This exception is raised by another thread calling killThread, or by the system if it needs to terminate the thread for some reason.

So, using a special exception that is only raised by cancel as in the OPs solution seems like a better idea.

My use case for link and cancel on the same thread may simply be bad design, but I'm not sure yet: right now I'm debugging some code I didn't write that is dropping important exceptions. It already uses cancel and I'm adding link so that important exceptions won't be lost. After I understand the problem I may be able to rewrite it to avoid link + cancel.

@ntc2
Copy link
Contributor

ntc2 commented Nov 30, 2017

I doubt this matters in practice very often, but I believe there's a race between forking the Async and setting up the ThreadKilled handler. To avoid the race, something like the following might be better:

asyncLinked ::  IO () -> IO (Async ())
asyncLinked action = do
  mask $ \restore -> do
  a <- async $ handle (ThreadKilled -> return ()) (restore action)
  restore $ do
  U.link a
  return a

Re my use case, what I want is a program that fails fast when any of its threads fail with an uncaught exception. With the default behavior of unlinked Asyncs the exceptions in child threads get lost. Presumably I could restructure my program so that every thread is waited on, but just linking seems simpler, as long as it doesn't stop me from canceling. Hence asyncLinked.

Why not change cancel to send a custom exception (e.g. Control.Concurrent.Async.ThreadCancelled) that's specific to Control.Concurrent.Async, instead of reusing Control.Exception.ThreadKilled, and make link ignore this custom exception? I expect it's uncommon that people want to kill the parent thread intentionally by canceling a linked child thread, and if they do want that behavior they could always use cancelWith a Control.Exception.ThreadKilled.

@simonmar
Copy link
Owner

simonmar commented Dec 5, 2017

This is now done, see b015e01

@simonmar simonmar closed this as completed Dec 5, 2017
@ntc2
Copy link
Contributor

ntc2 commented Dec 5, 2017

Great! To help future code archeologists, commits relevant to fixing this issue (and #64) included

  • df39a15: wrap exceptions rethrown by link in new async exception wrapper type ExceptionInLinkedThread.
  • fd963dc: make cancel throw new AsyncCancelled instead of ThreadKilled.
  • b015e01: add linkOnly and link2Only that take a predicate specifying which exceptions in the linked thread should be propagated to the parent, and change link and link2 to no longer propagate the exception raised by cancel (now AsyncCancelled).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants