Fix treatment of async exceptions

[edsko]

In #92 we added an exception handler that was meant to catch all exceptions (sync and async). This got changed in #114 (specifically, 52a9619): when we moved from Control.Exception to UnliftIO.Exception, we got a different behaviour for catch and friends (see well-typed/grapesy#193 (comment)) for a full list. This commit fixes some unintended consequences of this change.

I tried to do an exhaustive check of all uses of these functions in http2, I'll post a full report separately.

[edsko]

I checked all uses of catch and friends (all functions that ignore async exceptions in unliftio but not in base) in http2:

• Network.HPACK.HeaderBlock.Decode. Catches BufferOverrun specifically, which is thrown synchronously. Unproblematic.

• Network.HPACK.HeaderBlock.Encode. Also BufferOverrun.

• Network.HTTP2.H2.HPACK. Catches DecodeError, which is thrown synchronously. Unproblematic.

• Network.HTTP2.Server.Run. Use in stopAfter is wrong: if the server killed because some external thread kills it with an async exceptions, we definitely do want to run the cleanup handler. Replaced try by trySyncOrAsync, and changed both the signature and the documentation to clarify intend.

• Network.HTTP2.H2.Manager. The use of catch here specifically also wants to catch asynchronous exceptions (see Don't leak exceptions from managed threads #92). Therefore replaced it with catchSyncOrAsync.

• Network.HTTP2.H2.Sender, frameSender main body. We have

  (loop 0 `E.finally` setSenderDone) `E.catch` wrapException

  I'm not entirely sure here what we want to do; probably not catching async exceptions is the right thing: any exception that we raise in the frame sender gets wrapped in BadThingHappen, but if some external thread sends us an async exception, then it bubbles up unwrapped. @kazu-yamamoto I've therefore left this one unchanged.

• Network.HTTP2.H2.Sender, outputOrEnqueueAgain. Here we are catching e :: SomeException (passing to resetStream), which closes the stream with reason ResetByMe e -- and then does not rethrow the
  exception. So the intention here almost certainly is to only catch exceptions thrown explicitly in the scope of the call to handle, not async exceptions: this call is correct.

• Network.HTTP2.H2.Receiver. Here we catch SomeException, and when we do, we call sendGoAway, which enqueues a CFinish instruction. It's not entirely clear to me what the intention is: if we throw, say, KillThread, to the client, so we want that exception to be caught and result in a CFinish instruction? If so, we probably don't want to swallow the exception, as it's done now; so it's probably fine as-is. @kazu-yamamoto I've therefore also left this one unchanged.

I also checked time-manager, which contains this twice:

onTimeout `E.catch` ignoreAll

The intention here is I think to ignore exceptions thrown by the timeout handler itself, so it's indeed correct to not catch async exceptions here. Left this one unchanged also.

[kazu-yamamoto]

After discussing with @khibino some days ago, I began to think that asynchronous exceptions are a bad pattern in Haskell network programming.
I should think whether or not I can remove asynchronous exceptions from http2.

[edsko]

I think the test failure we're seeing is unrelated to this PR. I can produce it on my machine on main also:

HTTP2.Server
  server
    handles normal cases [ ]

(then timeout). It happens only very rarely, but it does happen. I'm also seeing these show up from time to time, on main:

  test/HTTP2/ServerSpec.hs:47:9: 
  1) HTTP2.Server.server handles normal cases
       uncaught exception: IOException of type NoSuchThing
       Network.Socket.connect: <socket: 13>: does not exist (Connection refused)

  To rerun use: --match "/HTTP2.Server/server/handles normal cases/" --seed 148294499

[edsko]

Have rebased on latest main.

[edsko]

I should think whether or not I can remove asynchronous exceptions from http2.

Asynchronous are indeed notoriously difficult to deal with. Removing them from http2 completely would be difficult however: how are you going to kill worker threads when they time out, or tell them that the client has disappeared? I suppose you could them an (T)MVar to poll, and leave it their responsibility. Indeed, in grapesy I do something along these lines: I run the worker in separate threads, that http2 is unaware of, and the main thread just sits there waiting for either the main thread to terminate or http2 to throw a (KilledByHttp2ThreadManager) exception.

This feels like quite a large design departure (though a compat shim could be provided that just spawns an additional thread, waitings on the (T)MVar, and kills the main thread when told to, I guess).

[kazu-yamamoto]

Threads should check STM in the beginning of each loop.
They can check if their sockets are ready for reading:

import Control.Concurrent
import Control.Concurrent.STM
import System.Posix.Types

checkReadAvailable :: Socket -> IO (STM (), IO ())
checkReadAvailable s = withFdSocket s $ \fd -> threadWaitReadSTM $ Fd fd

Timeout can be implemented with SystemTimerManager.

[kazu-yamamoto]

Thinking this issue for a day, I decided:

(1) I will merge #137 and #138 as a workaround
(2) Then I will get rid of asynchronous exceptions someday

[edsko]

Yes, a polling setup like you describe is possible.
For the timer manager, do you mean https://hackage.haskell.org/package/base-4.20.0.1/docs/GHC-Event.html#t:TimerManager ? If so, I guess we could, and then register an action that writes to an another STM variable, so that you can poll that also.

[edsko]

Thinking this issue for a day, I decided:

(1) I will merge #137 and #138 as a workaround (2) Then I will get rid of asynchronous exceptions someday

Ok, that works for me :) If and when you have a PR that removes all async exceptions, feel free to ping me to try it out with grapesy.

[kazu-yamamoto]

For the timer manager, do you mean https://hackage.haskell.org/package/base-4.20.0.1/docs/GHC-Event.html#t:TimerManager ?

Yes. @khibino and I actually use it in dnsext libraries.

[kazu-yamamoto]

If and when you have a PR that removes all async exceptions, feel free to ping me to try it out with grapesy.

Thanks. I will ping you!

[kazu-yamamoto]

@edsko #137 has been merged.
Unfortunately, #138 cannot be merged straightforwardly.
Would you resolve conflicts and rebase this PR onto the current main?

[edsko]

Yup, will do!

[edsko]

@kazu-yamamoto I have rebased (and also ran fourmolu). Let me just try this out with the grapesy test suite also before merging.

[edsko]

Ok, the grapesy test suite says all is fine :) (well-typed/grapesy#196). I think this is good to go!

[kazu-yamamoto]

LGTM

[kazu-yamamoto]

Merged.
A new version has been released.
Thanks!

[edsko]

Thanks @kazu-yamamoto !