Async.cancel does not work inside of withException

[parsonsmatt]

Related: #74

This is a minimal reproduction of the issue we're seeing.

  describe "withException" $ do
    it "should work when withAsync is in the handler" $ do
      let
        action =
          error "oops"
            `onException` do
              let
                timerAction n = do
                  threadDelay 1000000
                  when (n < 10) $ do
                    timerAction (n + 1)
              withAsync (timerAction 0) $ \a -> do
                cancel a
      action
        `shouldThrow`
          errorCall "oops"

This should finish instantaneously, but it actually waits for timerAction to complete - all 10 seconds of it. cancel is therefore broken inside of a withException or onException or even a bracket call.

What's very surprising about this behavior is that catch doesn't share it. catch works exactly like you'd expect, even though a very simple implementation of withException is withException action handler = catch action (\e -> handler e >> throwIO e).

[parsonsmatt]

One solution here is to use withAsyncWithUnmask (\unmask -> unmask (timerAction 0)) $ \a -> ....

It makes me wonder if the UnliftIO.Async module needs to care about the masking state of the program, since much of the Async API requires async exceptions to work appropriately.

For example, if withAsync detects that it is in a MaskedUninterruptible state, maybe it calls withAsyncWithUnmask under the hood. Or maybe it uses unsafeUnmask on the receiving thread.

[snoyberg]

This looks like correct behavior for withException to me, but it's convoluted. There's been a long history to the discussion of how cleanup actions should behave with regard to masking state. I don't remember where the original discussions happened, but I'm in the camp that the only correct behavior is to have exceptions masked in cleanups. That's because, otherwise, exceptions can "leak in" before you have the ability to mask. Also, as far as a default goes, masking is likely the better default. You typically want to ensure that cleanup operations can run to completion.

You might be right about the change to the Async behavior, but unless I'm mistaken, we take that behavior directly from the async package itself.