Give spawned threads control over async exceptions

[edsko]

@kazu-yamamoto , I'm not 100% sure if I got this right, because I don't yet have a complete picture of all the control flow in http2.

http2 spawns various threads on behalf of the client code. The main idea is of this PR is to give that client code a way to handle asynchronous exceptions; specifically, to make sure that they have an exception handler of their own installed before async exceptions are enabled.

The key change is this one:

data OutBody = 
    .. 
    -- | Streaming body takes an unmask function, a write action and a flush action.
  | OutBodyStreaming ((forall a. IO a -> IO a) -> (Builder -> IO ()) -> IO () -> IO ())

Client side

On the client side, I am fairly confident that what I have is right. I have modified forkManaged (from #74) to not call unmask directly, but instead pass it as an argument to the thread, so that it can decide to unmask exceptions if and where it wants; we then just pass that on as an argument to the function in the OutBodyStreaming.

Server side

On the server side, I am a bit less certain. Here threads are spawned by the manager; so I have changed the Action type to

newtype Action = Action { runAction :: (forall a. IO a -> IO a) -> IO () }

I then propagate the unmask argument through, and in response pass it to the function inside OutBodyStreaming; for all other calls, I just call unmask directly (this is important, otherwise timeouts would not be possible).

What I am unsure about is if this limits the scope of where async exceptions are enabled too much; in particular, what about the rest of go in worker?

Alternative approach

So far, I have only really needed this on the client, I haven't started on the server side implementation of gRPC yet; so I'm not sure if this change is even useful on the server side (mostly since those threads seem to do more than just execute user code). I suspect that it is, but I don't have hard evidence for it currently.

The only reason I made the server modifications at this point is that the client and the server share the OutBody type. So an alternative approach would be to decouple these, and allow the client and the server to diverge in the type of the handler; those it's nice of course to have the consistency.

Finally, if you think that this whole thing is not a good idea and we should not give the user this control, then feel free to close the PR again; it allows me to make my async exception handling more airtight, but I don't depend on it critically.

(BTW, haven't forgotten about #76 , I will get to it soon.)

[kazu-yamamoto]

My impression is that the change of OutBodyStreaming is drastic.
If we add a field to specify exception handlers in ClientConfig, is your requirement satisfied?

[edsko]

A global exception handler like that would need to be passed an argument in order to be able to distinguish one context from another, and then I guess that context should be passed as an argument to OutBodyStreaming instead of the unmask handler. And then it's not totally clear what the type of that context should be, so we'd need a type parameter or something? Or did you have something else in mind?

[kazu-yamamoto]

Would you show examples of the usage of the first argument?
I know some examples exit in this PR but they are just called at the beginning.

[kazu-yamamoto]

Perhaps, it is a good idea to add a new constructor like OutBodyStreamingUmask.

[edsko]

Perhaps, it is a good idea to add a new constructor like OutBodyStreamingUmask.

I'd be fine with that. I will give you an example of how I am using this, just in the middle of some other things at the moment. I will get back to you shortly :)

[epoberezkin]

If we add a field to specify exception handlers in ClientConfig, is your requirement satisfied?

it would indeed be hard to use, as ClientConfig is constructed once, and the exceptions need to be handled when the request is sent... This is somewhat over my head :), but from the user's point of view I just want to be to catch exceptions thrown in a streaming function where I send the request.

[edsko]

So let me sketch the way I would want to use this. As I mentioned previously, I am working on a gRPC library, based on http2. The only http2 request type I use is requestStreaming, with two threads: one is responsible to sending messages to the peer (this thread is started under the hood by http2), and one that I spawn myself for receiving message from the peer. Just like @epoberezkin , I want to catch exceptions that happen in the context of this call, so that if something goes wrong, the user of the library gets detailed information what went wrong and where.

This is tricky to get completely right, so I have written a new abstraction to help me with this; see https://github.com/well-typed/grapesy/blob/main/src/Network/GRPC/Util/Thread.hs if you'd like to see the details. It's a bit like async, although not quite the same; but the idea is similar: we have a TVar that is keeping track of the state of the thread (one complication to take care of here, unlike in async, is that I'm not even sure if the thread will be started at all: the thread that is started under the hood by http2 might not be started at all if the request could not be started). I then keep track of two such TVars in the abstraction around an ongoing gRPC call (https://github.com/well-typed/grapesy/blob/4e9de99262cc47d95fac88257eb6193d92b8f18e/src/Network/GRPC/Client/Call.hs#L60); this means that the state of the threads of the call are always inspectable; if I try to send something and the thread that is meant to handle it has died, an exception will be raised at that point; no exceptions get swallowed, and they are always propagated to where they are relevant. There is a detailed discussion of the control flow at https://github.com/well-typed/grapesy/blob/4e9de99262cc47d95fac88257eb6193d92b8f18e/src/Network/GRPC/Client/Call.hs#L115-L165 .

The way that this should ideally work is something like this:

mask_ $ forkWithUnmask $ \unmask ->   -- this bit lives in http2
   -- this bit lives in my code :
  threadBody tvar ... $ unmask $ do 
     -- do whatever needs to happen in this request

This would ensure that all exceptions are disabled until we are safely instead the body of threadBody, which would then ensure that all exceptions are caught and stored in the relevant TVar, so that they are part of the Call state. However, without this PR, this is not possible; my current code likes like this instead:

forkIO $ -- this lives in http2  
   -- this bit lives in my code :
  mask $ \unmask -> 
    threadBody tvar ... $ unmask $ do 
       -- do whatever needs to happen in this request

Almost the same, but not quite: there is a gap between that the thread gets started by http2 and that my infrastructure kicks in for catching exceptions; if the thread is killed in that gap (due to an async exception coming from somewhere), it will not be caught, the thread state TVar will not be updated, and I have lost control of the situation: the thread will be dead, I will not be able to send any more messages over the request, but I can't see what's happening and can't report a good exception to the user.

It's true that I'm being a bit pedantic here, as there is no real reason to assume that an async exception will be delivered to the thread in that gap. But I don't like solutions that work most of the time; I prefer a solution that works all of the time :) Having the unmask handler be passed as an argument makes it much easier for me to reason about my code: I know the thread will be started with async exceptions masked, so the only exceptions i need to worry about arise from calls that I'm making, which I can wrap in exception handlers of their own; and I know when I'm calling unmask, so I can make sure to call that only once an exception handler is in place.

[edsko]

Obsoleted by #80.

[epoberezkin]

@kazu-yamamoto am I right that this is now fixed in 4.2? I was trying to migrate to it but it seems not as straightforward with all the changed types...

[kazu-yamamoto]

@epoberezkin The answer is yes.

[kazu-yamamoto]

@epoberezkin Off topic but I'm sure that you are interested in haskellfoundation/tech-proposals#57

[kazu-yamamoto]

@epoberezkin See 8871654. Its test code is an example of migration.