catchSyncOrAsync doesn't catch SyncExceptionWrapper

[parsonsmatt]

EDIT: the text/title of this issue are confused - I missed that the test that could see-through the wrapper was actually catching SomeException and doing fromExceptionUnwrap on it.

This test illustrates the issue:

    it "should catch unliftio-wrapped sync exceptions" $ do
      result <- (`catchSyncOrAsync` return) $ throwIO Control.Exception.ThreadKilled
      result `shouldBe` Control.Exception.ThreadKilled

This test currently fails:

Failures:

  test/UnliftIO/ExceptionSpec.hs:60:7: 
  1) UnliftIO.Exception.catchSyncOrAsync should catch unliftio-wrapped async exceptions
       expected: Just Exception1
        but got: Nothing

  To rerun use: --match "/UnliftIO.Exception/catchSyncOrAsync/should catch unliftio-wrapped async exceptions/"

We're doing throwIO ThreadKilled, which throws SomeException (SyncExceptionWrapper ThreadKilled).

Meanwhile, catchSyncOrAsync is not looking "through" the SyncExceptionWrapper:

catchSyncOrAsync :: (MonadUnliftIO m, Exception e) => m a -> (e -> m a) -> m a
catchSyncOrAsync f g = withRunInIO $ \run -> run f `EUnsafe.catch` \e -> run (g e)

[snoyberg]

I don't think there's any reason this function should unwrap SyncExceptionWrapper, or any indication in the docs that it does so. It's simply catching anything, including wrappers. Though I'm confused by the exact output of this test case, something looks fishing about the Maybe wrapping.

[parsonsmatt]

oh, lol, i was doing some debugging and may have changed some behavior - i'll correct the output

I think the odd thing to me is that the code does unwrap the AsyncExceptionWrapper. I would expect symmetry there.

[parsonsmatt]

OK, new test failure output:

Failures:

  test/UnliftIO/ExceptionSpec.hs:61:5: 
  1) UnliftIO.Exception.catchSyncOrAsync should catch unliftio-wrapped sync exceptions
       uncaught exception: SyncExceptionWrapper
       thread killed

  To rerun use: --match "/UnliftIO.Exception/catchSyncOrAsync/should catch unliftio-wrapped sync exceptions/"

And test suite code (with other examples, for symmetry):

  describe "catchSyncOrAsync" $ do
    it "should catch sync exceptions" $ do
      result <- (`catchSyncOrAsync` return) $ throwIO Exception1
      result `shouldBe` Exception1
    it "should catch async exceptions" $ do
      result <- withAsyncExceptionThrown $ \m -> m `catchSyncOrAsync` return
      result `shouldBe` cancelled
    it "should catch unliftio-wrapped async exceptions" $ do
      result <- withWrappedAsyncExceptionThrown $ \m -> m `catchSyncOrAsync` return
      fromExceptionUnwrap result `shouldBe` Just Exception1
    it "should catch unliftio-wrapped sync exceptions" $ do
      result <- (`catchSyncOrAsync` return) $ throwIO Control.Exception.ThreadKilled
      result `shouldBe` Control.Exception.ThreadKilled

So it catches sync-thrown-as-sync exceptions, it catches async-thrown-as-async exceptions, it catches sync-thrown-as-async exceptions, and it fails to catch async-thrown-as-sync exceptions.

...

Oh.

The wrapped one is using fromExceptionUnwrap.

The test suite Works if I do that:

    it "should catch unliftio-wrapped async exceptions" $ do
      result <- withWrappedAsyncExceptionThrown $ \m -> m `catchSyncOrAsync` return
      fromExceptionUnwrap result `shouldBe` Just Exception1
    it "should catch unliftio-wrapped sync exceptions" $ do
      result <- (`catchSyncOrAsync` return) $ throwIO Control.Exception.ThreadKilled
      fromExceptionUnwrap result `shouldBe` Just Control.Exception.ThreadKilled

🤔

I think this behavior is a bit perplexing!

Would you consider a patch that changes behavior, such that catch is capable of "seeing through" a SyncExceptionWrapper, and catchSyncOrAsync is capable of "seeing through" either wrapper?

Here's my main motivation: ExceptionInLinkedThread. This has an Exception instance that uses SomeAsyncException, because we have two ways of throwing it:

1. link, which does an async throwTo tid (ExceptionInLinkedThread me (toException blah)
2. wait, which does a sync waitCatch async >>= throwIO . ExceptionInLinkedThread async

The async library (and UnliftIO.Async, which merely lifts it) uses Control.Exception, which does not use any wrapper. So catchSyncOrAsync can catch it. But if I rethrow from it (using a safe-exceptions or unliftio throwing function), then it will receive the wrapper, and then nothing upstream can catch it.

I think the behavior I'm expecting is a bit like this:

catch :: (Exception e) => IO a -> (e -> IO a) -> IO a
catch action handler =
  action `EUnsafe.catch` \(someExn :: SomeException) ->
    case fromException someExn of
      Just e 
        | isSyncException e -> handler e
        | otherwise -> 
            -- we caught an async exception without a wrapper
            -- rewrap it
            throwIO e
       Nothing ->
        -- is not a plain `e` - possibly a sync wrapped e?
        case fromException someExn of
          Just (SyncExceptionWrapper innerAsync) ->
            case fromException innerAsync of
               Just e -> 
                 -- The user is trying to catch an async exception
                 -- The SyncExceptionWrapper indicates that it was thrown as a sync exception
                 -- Safe to handle
                 handler e
               Nothing ->
                 -- We can rethrow here without rewrapping checks, because we're already in a SyncExceptionWrapper
                 EUnsafe.throwIO someExn
           Nothing ->
             -- The exception is not what we were expecting. toss it. 
             EUnsafe.throwIO someExn

[snoyberg]

Maybe this is simply a difference in the implementation of the Exception instance for AsyncException and SyncException? I'd feel pretty bad about changing the definition of catch, but less bad about unifying those two instances.

[parsonsmatt]

Hm - AsyncExceptionWrapper uses the SomeAsyncException constr, while SyncExceptionWrapper does not - it's a "regular" exception. This makes sense to me -

isSyncException (SyncExceptionWrapper ThreadKilled) = True
isSyncException ThreadKilled = False

I think the confusing thing, to me, is that doing something like:

action `catch` \(ExceptionInLinkedThread _ _) -> pure ()

won't ever catch ExceptionInLinkedThread, regardless if it is thrown sync or async.

Thrown from Control.Concurrent.Async.wait

wait does a Control.Exception.throwIO (ExceptionInLinkedThread a someExn), throwing a sync exception.

UnliftIO.Exception.catch will attach the exception handler, which we can expand a bit:

catch action handler = 
    action `EUnsafe.catchAny` \someException -> 
        case fromException someException of
            Nothing -> 
                -- Wrong type, catch doesn't even talk to it
                EUnsafe.throwIO someException
            Just exn ->
                -- From type inference, this means that we have the right type
                -- for `handler` to operate on it
                if isSyncException exn
                then 
                    -- `catch` only handles things that have an `Exception` instance
                    -- corresponding to a non-`SomeAsyncException` constructor
                    handler exn
                else
                    -- since ExceptionInLinkedThread always returns False for
                    -- isSyncException, we don't handle it.
                    EUnsafe.throwIO someException

What is IMO especially odd is that catch can never catch AsyncExceptionWrapper - even though that's explicitly the point of the wrapper! to mark an async exception as synchronously thrown.

Hmm.

[parsonsmatt]

What about this behavior?

  fdescribe "proposed catch" $ do
    let
      myCatch :: (MonadUnliftIO m, Exception e) => m a -> (e -> m a) -> m a
      myCatch action handler =
        withRunInIO $ \runInIO -> do
          runInIO action `Control.Exception.catch` \someException@(SomeException inner) -> do
            if isSyncException inner
            then
              case fromExceptionUnwrap someException of
                Just e ->
                  runInIO $ handler e
                Nothing ->
                  Control.Exception.throwIO someException
            else
              Control.Exception.throwIO someException

      myHandle :: (MonadUnliftIO m, Exception e) => (e -> m a) -> m a -> m a
      myHandle = flip myCatch

    it "catches sync exceptions" $ do
      myHandle (\Exception1 -> pure 'a') (throwIO Exception1)
        `shouldReturn` 'a'
    it "catches sync-wrapped async exceptions" $ do
      myHandle (\ThreadKilled -> pure 'a') (throwIO ThreadKilled)
        `shouldReturn` 'a'
    it "does not catch async exceptions" $ do
      myHandle (\ThreadKilled -> pure 'a') (Control.Exception.throwIO ThreadKilled)
        `shouldThrow` (ThreadKilled ==)
    it "does not catch async-wrapped sync exceptions" $ do
      myHandle (\Exception1 -> pure 'a') (Control.Exception.throwIO (AsyncExceptionWrapper Exception1))
        `shouldThrow` \(SomeException e) -> fromMaybe False $ do
          SomeAsyncException e <- cast e
          AsyncExceptionWrapper e <- cast e
          Exception1 <- cast e
          pure True

[snoyberg]

I'm honestly not sure I'd be comfortable with any change to behavior here, even if it makes things more coherent. These kinds of semantics are subtle and not checked by the compiler. A change like this may silently break code in lots of different code bases. I think the best approach would be a new function providing the specific functionality you're looking for. But overall, my real thought is that it's a mistake to use the same type for both sync and async exceptions and try to make the catching work in any meaningful way.