Opened 6 years ago

Last modified 20 months ago

#5902 new bug

Cannot tell from an exception handler whether the exception was asynchronous

Reported by: simonmar Owned by: simonmar
Priority: normal Milestone:
Component: Compiler Version: 7.4.1
Keywords: Cc: pho@…, exbb2@…, idhameed@…, hackage.haskell.org@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Following on from #2558 which was closed (by me) as wontfix, we still don't have a way to reliably tell whether an exception we caught was asynchronous or not. There are some suggestions in #2558, we just need to implement something. The fix for #3997 was defficient due to this, as exposed by the test program in #5866.

Change History (29)

comment:1 Changed 6 years ago by simonmar

Priority: highnormal

After playing around with this for a while I'm not sure there's anything sensible we can do. I tried passing a Bool to the handler in the primitive catch# to indicate whether the exception had been thrown asynchronously or not, but in the example I was interested in (unsafeInterleaveIO getLine, from #5866) it didn't help: the exception is first caught and thrown again by an inner exception handler before being caught by the outer exception handler. The inner exception handler is not in a position to re-throw the exception asynchronously because it has no sensible way to resume, but the outer handler can. In this case, checking whether the exception is one of the designated asynchronous exceptions (ThreadKilled etc.) works fine.

One positive step we could take is to use the exception hierarchy properly and make an AsyncException sub-hierarchy, and put Timeout under it. This would fix the specific problem in #5866.

comment:2 Changed 6 years ago by PHO

Cc: pho@… added

comment:3 Changed 5 years ago by igloo

Milestone: 7.6.17.6.2

comment:4 Changed 5 years ago by exbb2

Cc: exbb2@… added

comment:5 Changed 4 years ago by igloo

Milestone: 7.6.27.8.1
Priority: normalhigh

I think we've been running into problems with this when using the GHC API, with the same async exception being caught multiple times.

Here's a small testcase of what I think is going on:

import Control.Concurrent
import Control.Exception
import System.IO.Error
import System.IO.Unsafe

-- Our code:
main = do tid <- myThreadId
          forkIO $ do threadDelay 1000
                      throwTo tid UserInterrupt
          print i `catch` \e -> putStrLn ("Got " ++ show (e :: AsyncException))
          print i `catch` \e -> putStrLn ("Got " ++ show (e :: AsyncException))

-- A small simulation of the GHC API:
f :: IO Integer
f = (return $! sum [1..10000000]) `catchIOError` \_ -> return 3

i :: Integer
i = unsafePerformIO f

Although we only throw UserInterrupt once, we catch it twice:

$ ghc -O --make exc
$ ./exc
Got user interrupt
Got user interrupt

Unfortunately, I can't see a workaround if you want to use async exceptions with the GHC API. Once you throw one, there's a chance that an unsafePerformIO thunk somewhere inside the GHC API will end up containing it as a synchronous exception, and then any call to the GHC API that uses that thunk will just throw that exception.

comment:6 Changed 4 years ago by simonmar

Any time there's a catch/rethrow inside an unsafePerformIO we'll have this problem. So in the specific case of the GHC API it would help to know where this is happening, and whether we can avoid it.

One way I can think of that might help in general is to have a special kind of catch that doesn't catch asynchronous exceptions. So in cases like your example, catchIOError would use the special sync-only catch internally, and would ignore the async exception.

However, this unfortunately doesn't help with bracket and finally, which need to do some cleanup in the exception handler, and I suspect these are more common than catch.

comment:7 Changed 4 years ago by edsko

For the benefit of people who find this ticket: the SomeAsyncException class and corresponding functions are now in HEAD.

comment:8 Changed 4 years ago by simonpj

Priority: highnormal

Unlikely to happen for 7.8.1; we don't even know what to do!

Simon

comment:9 Changed 4 years ago by igloo

If we had

newCatch :: Exception e => IO a -> ((e, Bool{- async? -}) -> IO a) -> IO a
newCatch = ... primitive ...

newThrow :: Exception e => (e, Bool) -> a
newThrow (e, async) = if async then do t <- myThreadId
                                       throwTo t e
                               else throw e

then would using newCatch and newThrow in place of catch and throw solve the problem? (at the expense of having this annoying Bool getting in the way, and making the code less pretty).

comment:10 in reply to:  9 Changed 4 years ago by exbb2

Replying to igloo:

If we had

newCatch :: Exception e => IO a -> ((e, Bool{- async? -}) -> IO a) -> IO a
newCatch = ... primitive ...

newThrow :: Exception e => (e, Bool) -> a
newThrow (e, async) = if async then do t <- myThreadId
                                       throwTo t e
                               else throw e

then would using newCatch and newThrow in place of catch and throw solve the problem? (at the expense of having this annoying Bool getting in the way, and making the code less pretty).

I've had that design for quite some time now, I'm unsure if there's another sensible solution, except magically tagging the exception values themselves, but that could only complicate things.

comment:11 Changed 4 years ago by simonmar

It doesn't solve the problem, because the programmer has to be prepared for newThrow to return. This is what happens if an async exception is thrown to a thread that is inside newCatch inside unsafePerformIO inside a thunk evaluation, and the thunk is subsequently re-entered.

comment:12 Changed 4 years ago by simonmar

see also comment:1

comment:13 Changed 4 years ago by igloo

Hmm. If we also had something like

newUnsafePerformIO :: IO a -> a
newUnsafePerformIO io = unsafePerformIO io'
    where io' = io `newCatch` (\eb -> (newThrow eb >> io'))

so that at the outermost level the unsafePerformIO knows how to continue, then would that suffice? Or would it still be possible for problematic thunks inside io to be reentered?

comment:14 Changed 4 years ago by simonmar

Now I think about it, this isn't such a bad idea. Unfortunately we can't implement newCatch, because the async exception might have been re-thrown (say by a finally) inside the io. So the best you can do is recognise an async exception by its type - i.e. if it is an child of AsyncException then it's async, otherwise not. But that's good, because it means we don't have to change the RTS or primops.

Then there's the question of whether restarting the io is safe. It probably is; as Simon pointed out, the io inside unsafePerformIO is supposed to be a benign side-effect, so we ought to be able to repeat it.

comment:15 Changed 4 years ago by igloo

I don't see why you can't have newCatch, provide you are willing to also have newThrow and to deal with passing the annoying Bools around.

But using an AsyncException hierarchy instead might be OK too.

comment:16 Changed 4 years ago by simonmar

newCatch doesn't work for bracket. Consider:

bracket before after thing =
  mask $ \restore -> do
    a <- before
    r <- restore (thing a) `onException` after a
    _ <- after a
    return r

Now, when the async exception is received, onException runs the exception handler (after a), which cleans up whatever resource was allocated by before. But when this this resumed after the async exception, if onException is using newCatch/newThrow, it would re-run restore (thing a), which assumes that the resource is present and accessible.

So while it might make sense to re-run the whole unsafePerformIO when we resume after an async exception, it certainly doesn't make sense to do it at a finer granularity. We don't want most exception-handlers to be resumable.

comment:17 Changed 4 years ago by igloo

I'm a bit confused now. If we only use newUnsafePerformIO (and not plain unsafePerformIO), then is it still possible for a bracket to be resumed?

comment:18 Changed 4 years ago by exbb2

Priority: normalhigh

If a type-based solution is implemented, I propose for SomeException to be deprecated and split into SomeSyncException and SomeAsyncException.

Right now, SomeException and lack of discrimination for asynchronous exceptions pose serious threat to modularity in concurrent applications. Every innocuous catch or try anywhere at any time in any library can make a thread unintentionally uncancelable. I had to write patches for three different libraries because of this recurring problem; what would happen if my project depended on 50 libraries all throwing and catching exceptions?

comment:19 Changed 4 years ago by simonmar

@exbb2: we already have SomeAsyncException, and the asynchronous exceptions are children of this in the exception hierarchy. The exception hierarchy needs a single root, so SomeException remains.

Every innocuous catch or try anywhere at any time in any library can make a thread unintentionally uncancelable

I'm not sure what you mean here. The issue in this ticket only arises when using unsafePerformIO.

comment:20 in reply to:  19 Changed 4 years ago by exbb2

Replying to simonmar:

Every innocuous catch or try anywhere at any time in any library can make a thread unintentionally uncancelable

I'm not sure what you mean here. The issue in this ticket only arises when using unsafePerformIO.

tryNTimes n
 | n <= 0 = return ()
 | otherwise =
  forever (return ())
   `catch` \(_::SomeException) -> tryNTimes (n - 1)

or

try something :: IO (Either SomeException x)

Catching SomeException is sometimes reasonable, but it's unreasonable not to provide ability to discriminate against irrelevant asynchronous exceptions which do not directly follow from handled code.

comment:21 Changed 4 years ago by simonmar

Just never catch SomeException unless you intend to re-throw it. If you're handling an exception, you always know which exception(s) you're handling, so you can use type-specific handlers.

comment:22 in reply to:  21 Changed 4 years ago by exbb2

Replying to simonmar:

Just never catch SomeException unless you intend to re-throw it.

The code in my case quickly grew into monstrosity such as

bracket openFile doDangerousStuff close
 `catches`
  [\Async -> throw
  ,\Some  -> logFailureAndResume]

Even that doesn't work for Timeout, unless we resort to string comparison.

If you're handling an exception, you always know which exception(s) you're handling

No. All arguments and values in function can throw any exception whatsoever, there can be no knowing of what exceptions you'll receive. e.g. what if the next version of base introduces a hard-to-track division by zero?

We shouldn't ever care about what exception we got. If it had arisen from code enclosed by the handler, — there can be no doubt as to its intended recipient — and no danger in catching it. Asynchronous exceptions have only dimmest relation to code being executed or none at all, there can be no modularity when every exception handler is concerned with whole state of the universe.

comment:23 Changed 4 years ago by ihameed

Cc: idhameed@… added

comment:24 Changed 3 years ago by thoughtpolice

Milestone: 7.8.37.10.1

Bumping priority down (these tickets haven't been closely followed or fixed in 7.4), and moving out to 7.10 and out of 7.8.3.

comment:25 Changed 3 years ago by thoughtpolice

Priority: highnormal

Actually dropping priority. :)

comment:26 Changed 3 years ago by liyang

Cc: hackage.haskell.org@… added

comment:27 Changed 3 years ago by thoughtpolice

Milestone: 7.10.17.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

comment:28 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:29 Changed 20 months ago by thomie

Milestone: 8.0.1
Note: See TracTickets for help on using tickets.