withHandle #1015
withHandle #1015
Conversation
This prevents forgetting calls of "cancel".
| -- | Registering a timeout action and unregister its handle | ||
| -- when the body action is finished. | ||
| withHandle :: Manager -> TimeoutAction -> (Handle -> IO a) -> IO a | ||
| withHandle mgr onTimeout action = |
There was a problem hiding this comment.
withHandle mgr onTimeout =
E.bracket (register mgr onTimeout) cancel| @@ -106,10 +106,6 @@ http2server label settings ii transport addr app h2req0 aux0 response = do | |||
| logResponse req st msiz | |||
| mapM_ (logPushPromise req) pps | |||
| Left e | |||
| -- killed by the local worker manager | |||
| | Just E.ThreadKilled <- E.fromException e -> return () | |||
There was a problem hiding this comment.
This is now not caught anymore, right? Is that intentional?
withHandleKillThread only catches the T.TimeoutThread
There was a problem hiding this comment.
I think this is very old code.
Currenlty, killThread is not used anywhere.
There was a problem hiding this comment.
But people can implement their own forking behaviour with settingsFork. So it COULD happen that a ThreadKilled is thrown to this thread.
Though now that I think about it, this is actually better, right? Since if someone would want to kill an HTTP thread that's handling a request, this would just ignore the signal. So I now also believe this is the right decision.
There was a problem hiding this comment.
Note that ThreadKilled was tried to be caught only in HTTP/2.
If we worry about it, we should catch it in HTTP/1.1, too.
| -- unregister its handle when the body action is killed or finished. | ||
| withHandleKillThread :: Manager -> TimeoutAction -> (Handle -> IO ()) -> IO () | ||
| withHandleKillThread mgr onTimeout action = | ||
| E.handle handler $ E.bracket (registerKillThread mgr onTimeout) cancel action |
There was a problem hiding this comment.
Would it be better/worse to do the following? 🤔
E.bracket (registerKillThread mgr onTimeout) cancel $ E.handle handler action
There was a problem hiding this comment.
Interesting.
I don't know which is better.
There was a problem hiding this comment.
I don't think it will matter much... since the cancel will be run either way.
The difference is this, effectively:
-- Here's the body of 'bracket'
-- either here instead of:
-- mask $ \restore -> do
E.handle handler $ mask $ \restore -> do
a <- before
-- or here instead of:
-- r <- restore (thing a) `onException` after a
r <- restore (E.handle handler $ thing a) `onException` after a
_ <- after a
return r
|
Thank you for reviewing. |
This implements rule 2 of #1014.
Relating to #1013.