withAsync hangs if called during uninterruptibleMask

[gromakovsky]

Consider the following example:

import Control.Exception
import Control.Concurrent
import Control.Concurrent.Async

main = uninterruptibleMask_ (withAsync (threadDelay maxBound) (const $ return ()))

This program hangs forever.
As documentation states, withAsync behaves like withAsync action inner = bracket (async action) uninterruptibleCancel inner. In this case uninterruptibleCancel hangs because the thread which it's trying to kill has MaskedUninterruptible masking state.
I am not sure if it's supposed to work this way. Maybe it's a bug.
Note that it has the same effect for race_ and concurrently_ functions. Both examples below hang:

main = uninterruptibleMask_ (race_ (threadDelay maxBound) (return ()))
main = uninterruptibleMask_ (concurrently_ (threadDelay maxBound) (error "foo"))

There is a simple workaround for it:

main = uninterruptibleMask_ (withAsyncWithUnmask (\unmask -> unmask $ threadDelay maxBound) (const $ return ()))

This program doesn't hang. Shouldn't it be the default behaviour? I. e. shouldn't withAsync change masking state of spawned thread automatically?

[simonmar]

This behaviour makes sense to me, and it's consistent with the documentation. The user is requesting to mask asynchronous exceptions, and that's exactly what they get.

Do you really want to call async operations inside uninterruptibleMask? Why?

[gromakovsky]

Do you really want to call async operations inside uninterruptibleMask? Why?

I agree that it should be generally avoided, but my intuition is that withAsync should finish as soon as inner action passed to it finishes, even if for some reason (maybe by accident) it's used inside uninterruptibleMask (or just mask). I can quote @snoyberg about it:

My thinking is that exceptions sent to the parent thread have no effect on the child thread, and therefore a parent saying "don't interrupt me" doesn't tell us what the child really wants.

Note that it's also possible with mask_ (not uninterruptible one) as long as we don't use interruptible actions. Example:

main = mask_ (withAsync (print $ fix (* 2)) (const $ return ()))

And a more real life example:

-- | Run action and print warning if it takes more time than expected.
logWarningLongAction
    :: forall m a. CanLogInParallel m
    => Bool -> WaitingDelta -> Text -> m a -> m a
logWarningLongAction secure delta actionTag action =
    withAsync (waitAndWarn delta) (const action)
  where
    printWarning t = logWarning $
        sformat ("Action `"%stext%"` took more than "%shown) actionTag t

    waitAndWarn =
        let waitLoop acc = do
                threadDelay delta
                printWarning acc
                waitLoop (acc + s)
        in waitLoop s

main :: IO ()
main = bracket_ before after doSomething
  where
    -- We suspect that one of these may be slow due to some bug and want to check it.
    before = logWarningLongAction acquireResource
    after = logWarningLongAction releaseResource

So logWarningLongAction spawns a thread and periodically prints warnings about action taking too much time. In main we call acquireResource and releaseResource and suspect that they are buggy and decide to use logWarningLongAction with them (e. g. temporarily). If bracket_ comes from safe-exceptions package, then it will apply uninterruptibleMask to after and then it will never finish.

[simonmar]

This question is surprisingly subtle.

My thinking is that exceptions sent to the parent thread have no effect on the child thread, and therefore a parent saying "don't interrupt me" doesn't tell us what the child really wants.

So my initial reaction is that "this doesn't match the semantics of mask", which is that child threads inherit the masking state of the parent. That's a simple rule and easy to understand, and there's at least one good reason to do it like this: otherwise there's no way to create a thread in a masked state, which is really (really) important for providing guarantees about your child thread's behaviour. Note that you don't get any guarantees from the fact that nobody else knows the child thread's ThreadId, because there are async exceptions that arise spontaneously,: StackOverflow and HeapOverflow in particular

However, I can see the argument that in many cases having masking be inherited is not useful behaviour.

• withAsync relies on being able to kill the child
• When using withAsync, An async exception sent to the parent causes an async exception to be sent to the child, unless the parent is inside mask. This is true regardless of whether the child is masked or not; so in other words, having the child inherit the masking state doesn't help, because the parent's masking state already controls whether the exception gets sent to the child.

Take these two examples

a `finally` (b >> c)

and

a `finally` (b `concurrently` c)

Would you expect these two to behave the same with respect to async exceptions? I think it would be nice if we could say "yes". That's one of the goals of async, to make concurrency transparent and consistent. But if we make withAsync unmask the child, then in the second example, StackOverflow could be raised by b or c, whereas it wouldn't in the first example.

Furthermore, we would like to implement concurrently using only one extra thread (currently it uses two), but switching to non-inheriting behaviour would make this very difficult to implement, because one of the two computations would be running in the context of the parent thread.

So at the moment I'm tempted to conclude that you should use withAsyncWithUnmask in cases where you rely on being able to kill the child thread. But I'm aware that this could be somewhat surprising.

Similarly, we might ask why

a `finally` (timeout 3 cleanup)

doesn't work. Is this surprising? Perhaps - but would we then be surprised if

a `finally` (timeout 3 cleanup1 `concurrently` timeout 3 cleanup2)

worked? Shouldn't these either both work or both not work?

[simonmar]

I'll close this for now, feel free to re-open if you still think we should do something different here.

[parsonsmatt]

I just ran into this. fpco/unliftio#96

The UnliftIO.Exception module uses uninterruptibleMask in the handler of bracket, onException, and withException. This means any withAsync call that's used in exception cleanup cannot work.

This is somewhat tricky, because an idiom that we use somewhat often is bracket getThing putThing \thing ..., where putThing is not some resource-intensive, high-performance, 100% guarantee requirement cleanup handler - it's just something we want to happen afterwards. I implemented a forever timer thread to record metrics on how long we're waiting on database connections, which spins forever now if we call to the database in any of these functions.

For a real example in code, consider this function which makes a record available in the database for a test:

withRecord rec test = 
  bracket (runDB $ insert rec) (\id -> runDB $ delete id) test

The change I proposed in the UnliftIO.Async module is here:

withAsync :: MonadUnliftIO m => m a -> (Async a -> m b) -> m b
withAsync a b = withRunInIO $ \run -> do
  maskingState <- E.getMaskingState
  case maskingState of
    E.MaskedUninterruptible ->
      A.withAsyncWithUnmask (\unmask -> unmask (run a)) (run . b)
    _ ->
      A.withAsync (run a) (run . b)

I think this brings up the possible points of confusion you mention:

a `finally` (timeout 3 cleanup)
a `finally` (timeout 3 cleanup1 `concurrently` timeout 3 cleanup2)
a `finally` (b >> c)
a `finally` (b `concurrently` c)

All of these are indeed difficult, but the issue that i ran into is that:

a `finally`
  race
    (forever $ threadDelay 100 >> putStrLn "still waiting...")
    cleanup

will never terminate! The entire point of race is that the shortest action wins, and this is broken.