Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

withHandle #1015

Merged
merged 4 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions time-manager/ChangeLog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# ChangeLog for time-manager

## 0.1.3

* Providing `withHandle` and `withHandleKillThread`.

## 0.1.2

* Holding `Weak ThreadId` to prevent thread leak again
Expand Down
83 changes: 52 additions & 31 deletions time-manager/System/TimeManager.hs
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,20 @@ module System.TimeManager (
withManager,
withManager',

-- ** Registration
register,
registerKillThread,
-- ** Registering a timeout action
withHandle,
withHandleKillThread,

-- ** Control
tickle,
cancel,
pause,
resume,

-- ** Low level
register,
registerKillThread,
cancel,

-- ** Exceptions
TimeoutThread (..),
) where
Expand Down Expand Up @@ -96,6 +100,22 @@ killManager = reaperKill

----------------------------------------------------------------

-- | 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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withHandle mgr onTimeout =
    E.bracket (register mgr onTimeout) cancel

E.bracket (register mgr onTimeout) cancel action

-- | Registering a timeout action of killing this thread and
-- 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better/worse to do the following? 🤔

E.bracket (registerKillThread mgr onTimeout) cancel $ E.handle handler action

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting.
I don't know which is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

where
handler TimeoutThread = return ()

----------------------------------------------------------------

-- | Registering a timeout action.
register :: Manager -> TimeoutAction -> IO Handle
register mgr !onTimeout = do
Expand All @@ -105,6 +125,34 @@ register mgr !onTimeout = do
reaperAdd mgr h
return h

-- | Removing the 'Handle' from the 'Manager' immediately.
cancel :: Handle -> IO ()
cancel (Handle mgr _ stateRef) = do
_ <- reaperModify mgr filt
return ()
where
-- It's very important that this function forces the whole workload so we
-- don't retain old handles, otherwise disasterous leaks occur.
filt [] = []
filt (h@(Handle _ _ stateRef') : hs)
| stateRef == stateRef' = hs
| otherwise =
let !hs' = filt hs
in h : hs'

----------------------------------------------------------------

-- | The asynchronous exception thrown if a thread is registered via
-- 'registerKillThread'.
data TimeoutThread = TimeoutThread
deriving (Typeable)

instance E.Exception TimeoutThread where
toException = E.asyncExceptionToException
fromException = E.asyncExceptionFromException
instance Show TimeoutThread where
show TimeoutThread = "Thread killed by timeout manager"

-- | Registering a timeout action of killing this thread.
-- 'TimeoutThread' is thrown to the thread which called this
-- function on timeout. Catch 'TimeoutThread' if you don't
Expand All @@ -121,40 +169,13 @@ registerKillThread m onTimeout = do
Nothing -> return ()
Just tid' -> E.throwTo tid' TimeoutThread

-- | The asynchronous exception thrown if a thread is registered via
-- 'registerKillThread'.
data TimeoutThread = TimeoutThread
deriving (Typeable)

instance E.Exception TimeoutThread where
toException = E.asyncExceptionToException
fromException = E.asyncExceptionFromException
instance Show TimeoutThread where
show TimeoutThread = "Thread killed by timeout manager"

----------------------------------------------------------------

-- | Setting the state to active.
-- 'Manager' turns active to inactive repeatedly.
tickle :: Handle -> IO ()
tickle (Handle _ _ stateRef) = I.writeIORef stateRef Active

-- | Removing the 'Handle' from the 'Manager' immediately.
cancel :: Handle -> IO ()
cancel (Handle mgr _ stateRef) = do
_ <- reaperModify mgr filt
return ()
where
-- It's very important that this function forces the whole workload so we
-- don't retain old handles, otherwise disasterous leaks occur.
filt [] = []
filt (h@(Handle _ _ stateRef') : hs)
| stateRef == stateRef' =
hs
| otherwise =
let !hs' = filt hs
in h : hs'

-- | Setting the state to paused.
-- 'Manager' does not change the value.
pause :: Handle -> IO ()
Expand Down
2 changes: 1 addition & 1 deletion time-manager/time-manager.cabal
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Name: time-manager
Version: 0.1.2
Version: 0.1.3
Synopsis: Scalable timer
License: MIT
License-file: LICENSE
Expand Down
4 changes: 0 additions & 4 deletions warp/Network/Wai/Handler/Warp/HTTP2.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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 ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now not caught anymore, right? Is that intentional?

withHandleKillThread only catches the T.TimeoutThread

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is very old code.
Currenlty, killThread is not used anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-- killed by the local timeout manager
| Just T.TimeoutThread <- E.fromException e -> return ()
| isAsyncException e -> E.throwIO e
| otherwise -> do
S.settingsOnException settings (Just req) e
Expand Down
5 changes: 1 addition & 4 deletions warp/Network/Wai/Handler/Warp/Run.hs
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ fork set mkConn addr app counter ii = settingsFork set $ \unmask -> do

-- We need to register a timeout handler for this thread, and
-- cancel that handler as soon as we exit.
serve unmask (conn, transport) = E.bracket register cancel $ \th -> do
serve unmask (conn, transport) = T.withHandleKillThread (timeoutManager ii) (return ()) $ \th -> do
-- We now have fully registered a connection close handler in
-- the case of all exceptions, so it is safe to once again
-- allow async exceptions.
Expand All @@ -368,9 +368,6 @@ fork set mkConn addr app counter ii = settingsFork set $ \unmask -> do
-- Actually serve this connection. bracket with closeConn
-- above ensures the connection is closed.
when goingon $ serveConnection conn ii th addr transport set app
where
register = T.registerKillThread (timeoutManager ii) (return ())
cancel = T.cancel

onOpen adr = increase counter >> settingsOnOpen set adr
onClose adr _ = decrease counter >> settingsOnClose set adr
Expand Down
2 changes: 1 addition & 1 deletion warp/warp.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ library
stm >=2.3,
streaming-commons >=0.1.10,
text,
time-manager >=0.1 && <0.2,
time-manager >=0.1.3 && <0.2,
vault >=0.3,
wai >=3.2.4 && <3.3,
word8
Expand Down