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

Removing unliftio #1012

Merged
merged 6 commits into from
Nov 7, 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
6 changes: 3 additions & 3 deletions time-manager/System/TimeManager.hs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{-# LANGUAGE DeriveDataTypeable #-}
{-# LANGUAGE BangPatterns #-}
{-# LANGUAGE DeriveDataTypeable #-}

module System.TimeManager (
-- ** Types
Expand Down Expand Up @@ -29,11 +29,11 @@ module System.TimeManager (
) where

import Control.Concurrent (myThreadId)
import qualified Control.Exception as E
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main change because of this would be that AsyncExceptions will also be ignored if they are thrown during timeoutAction (whereas they wouldn't before)

So (I think?) this means a thread might be failed to be killed if the async exception is thrown while the timeoutAction is running?
So we might want to change to safe-exceptions instead of unliftio?

import Control.Reaper
import Data.IORef (IORef)
import qualified Data.IORef as I
import Data.Typeable (Typeable)
import qualified UnliftIO.Exception as E

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

Expand Down Expand Up @@ -144,7 +144,7 @@ cancel (Handle mgr _ stateRef) = do
| stateRef == stateRef' =
hs
| otherwise =
let !hs'= filt hs
let !hs' = filt hs
in h : hs'

-- | Setting the state to paused.
Expand Down
1 change: 0 additions & 1 deletion time-manager/time-manager.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ Extra-Source-Files: ChangeLog.md
Library
Build-Depends: base >= 4.12 && < 5
, auto-update >= 0.2 && < 0.3
, unliftio
Default-Language: Haskell2010
Exposed-modules: System.TimeManager
Ghc-Options: -Wall
42 changes: 18 additions & 24 deletions warp-tls/Network/Wai/Handler/WarpTLS.hs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ module Network.Wai.Handler.WarpTLS (
) where

import Control.Applicative ((<|>))
import Control.Exception (
Exception,
IOException,
SomeException (..),
bracket,
finally,
fromException,
handle,
handleJust,
onException,
throwIO,
try,
)
import Control.Monad (guard, void)
import qualified Data.ByteString as S
import qualified Data.ByteString.Lazy as L
Expand All @@ -73,6 +86,7 @@ import Network.Socket (
#endif
withSocketsDo,
)
import qualified Control.Exception as E
import Network.Socket.BufferPool
import Network.Socket.ByteString (sendAll)
import qualified Network.TLS as TLS
Expand All @@ -82,23 +96,7 @@ import Network.Wai.Handler.Warp
import Network.Wai.Handler.Warp.Internal
import Network.Wai.Handler.WarpTLS.Internal
import System.IO.Error (ioeGetErrorType, isEOFError)
import UnliftIO.Exception (
Exception,
IOException,
SomeException (..),
bracket,
finally,
fromException,
handle,
handleAny,
handleJust,
onException,
throwIO,
try,
)
import qualified UnliftIO.Exception as E
import UnliftIO.Concurrent (newEmptyMVar, putMVar, takeMVar, forkIOWithUnmask)
import UnliftIO.Timeout (timeout)
import System.Timeout (timeout)

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

Expand Down Expand Up @@ -323,12 +321,8 @@ mkConn
-> params
-> IO (Connection, Transport)
mkConn tlsset set s params = do
var <- newEmptyMVar
_ <- forkIOWithUnmask $ \umask -> do
let tm = settingsTimeout set * 1000000
mct <- umask (timeout tm recvFirstBS)
putMVar var mct
mbs <- takeMVar var
let tm = settingsTimeout set * 1000000
mbs <- timeout tm recvFirstBS
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes sense, but if we want to keep the same behaviour we'd have to change it to:

mbs <- unsafeUnmask $ timeout tm recvFirstBS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code before unsafeUnmask does not have unsafeUnmask.
Is it really necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

The umask in the diff is basically unsafeUnmask. But as you described earlier, that mkConn should be interruptable, this is unnecessary if the bracket (or whatever is used) is from Control.Exception instead of from UnliftIO.

Because the whole forkWithUnmask was a hack to get around the unliftio functions that mask more than necessary.

So no, I have changed my stance and adding unsafeUnmask is indeed not necessary.

case mbs of
Nothing -> throwIO IncompleteHeaders
Just bs -> switch bs
Expand Down Expand Up @@ -366,7 +360,7 @@ httpOverTls TLSSettings{..} set s bs0 params =
case mconn of
Nothing -> throwIO IncompleteHeaders
Just conn -> return conn
wrappedRecvN recvN n = handleAny (const mempty) $ recvN n
wrappedRecvN recvN n = handle (\(SomeException _) -> mempty) $ recvN n
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this will now also catch (and ignore) async exceptions.

backend recvN =
TLS.Backend
{ TLS.backendFlush = return ()
Expand Down
1 change: 0 additions & 1 deletion warp-tls/warp-tls.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ Library
, network >= 2.2.1
, streaming-commons
, tls-session-manager >= 0.0.4
, unliftio
, recv >= 0.1.0 && < 0.2.0
Exposed-modules: Network.Wai.Handler.WarpTLS
Network.Wai.Handler.WarpTLS.Internal
Expand Down
2 changes: 1 addition & 1 deletion warp/Network/Wai/Handler/Warp.hs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ module Network.Wai.Handler.Warp (

import Data.Streaming.Network (HostPreference)
import qualified Data.Vault.Lazy as Vault
import UnliftIO.Exception (SomeException, throwIO)
import Control.Exception (SomeException, throwIO)
Copy link
Contributor

Choose a reason for hiding this comment

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

This throwIO is fine if we're only throwing syncronous exceptions.

#ifdef MIN_VERSION_crypton_x509
import Data.X509
#endif
Expand Down
2 changes: 1 addition & 1 deletion warp/Network/Wai/Handler/Warp/Conduit.hs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@

module Network.Wai.Handler.Warp.Conduit where

import Control.Exception (assert, throwIO)
Copy link
Contributor

Choose a reason for hiding this comment

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

This throwIO is fine if we're only throwing syncronous exceptions.

import qualified Data.ByteString as S
import qualified Data.IORef as I
import Data.Word8 (_0, _9, _A, _F, _a, _cr, _f, _lf)
import UnliftIO (assert, throwIO)

import Network.Wai.Handler.Warp.Imports
import Network.Wai.Handler.Warp.Types
Expand Down
2 changes: 1 addition & 1 deletion warp/Network/Wai/Handler/Warp/FdCache.hs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import System.Posix.IO (
openFd,
setFdOption,
)
import UnliftIO.Exception (bracket)
import Control.Exception (bracket)
Copy link
Contributor

Choose a reason for hiding this comment

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

The UnliftIO bracket is different in that it uses uninterruptibleMask_ on the after actions. How important are the terminate actions?
They are cleaning up file descriptors, so that might be important enough to absolutely make sure it happens, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resouce-release actions in bracket (i.e. masked actions) are iterruptible only if they are blocked (such as takeMVar).
Non-blocking actions are NOT iterruptible.
In other words, asynchronous exceptions are not delivered to resource-release actions if they are non-blocking.

terminate is non-blocking, so it's free from asynchronous exceptions, I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes. Never mind. I've re-read the bracket docs, and it does. I misread, I think.

#endif
import System.Posix.Types (Fd)

Expand Down
12 changes: 6 additions & 6 deletions warp/Network/Wai/Handler/Warp/FileInfoCache.hs
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ module Network.Wai.Handler.Warp.FileInfoCache (
getInfo, -- test purpose only
) where

import Control.Exception (bracket, onException, throwIO)
Copy link
Contributor

Choose a reason for hiding this comment

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

UnliftIO.onException also runs with uninterruptibleMask. Making sure async exceptions wait until after the cleanup is done.

import Control.Reaper
import Network.HTTP.Date
#if WINDOWS
import System.PosixCompat.Files
#else
import System.Posix.Files
#endif
import qualified UnliftIO (bracket, onException, throwIO)

import Network.Wai.Handler.Warp.HashMap (HashMap)
import qualified Network.Wai.Handler.Warp.HashMap as M
Expand Down Expand Up @@ -58,7 +58,7 @@ getInfo path = do
, fileInfoDate = date
}
return info
else UnliftIO.throwIO (userError "FileInfoCache:getInfo")
else throwIO (userError "FileInfoCache:getInfo")

getInfoNaive :: FilePath -> IO FileInfo
getInfoNaive = getInfo
Expand All @@ -69,11 +69,11 @@ getAndRegisterInfo :: FileInfoCache -> FilePath -> IO FileInfo
getAndRegisterInfo reaper path = do
cache <- reaperRead reaper
case M.lookup path cache of
Just Negative -> UnliftIO.throwIO (userError "FileInfoCache:getAndRegisterInfo")
Just Negative -> throwIO (userError "FileInfoCache:getAndRegisterInfo")
Just (Positive x) -> return x
Nothing ->
positive reaper path
`UnliftIO.onException` negative reaper path
`onException` negative reaper path

positive :: FileInfoCache -> FilePath -> IO FileInfo
positive reaper path = do
Expand All @@ -84,7 +84,7 @@ positive reaper path = do
negative :: FileInfoCache -> FilePath -> IO FileInfo
negative reaper path = do
reaperAdd reaper (path, Negative)
UnliftIO.throwIO (userError "FileInfoCache:negative")
throwIO (userError "FileInfoCache:negative")

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

Expand All @@ -97,7 +97,7 @@ withFileInfoCache
-> IO a
withFileInfoCache 0 action = action getInfoNaive
withFileInfoCache duration action =
UnliftIO.bracket
bracket
(initialize duration)
terminate
(action . getAndRegisterInfo)
Expand Down
55 changes: 27 additions & 28 deletions warp/Network/Wai/Handler/Warp/HTTP1.hs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module Network.Wai.Handler.Warp.HTTP1 (
) where

import qualified Control.Concurrent as Conc (yield)
import Control.Exception (SomeException, catch, fromException, throwIO, try)
import qualified Data.ByteString as BS
import Data.Char (chr)
import Data.IORef (IORef, newIORef, readIORef, writeIORef)
Expand All @@ -17,8 +18,6 @@ import Network.Socket (SockAddr (SockAddrInet, SockAddrInet6))
import Network.Wai
import Network.Wai.Internal (ResponseReceived (ResponseReceived))
import qualified System.TimeManager as T
import UnliftIO (SomeException, fromException, throwIO)
import qualified UnliftIO
import "iproute" Data.IP (toHostAddress, toHostAddress6)

import Network.Wai.Handler.Warp.Header
Expand Down Expand Up @@ -115,7 +114,7 @@ http1server
-> Source
-> IO ()
http1server settings ii conn transport app addr th istatus src =
loop FirstRequest `UnliftIO.catchAny` handler
loop FirstRequest `catch` handler
where
handler e
-- See comment below referencing
Expand Down Expand Up @@ -151,7 +150,7 @@ http1server settings ii conn transport app addr th istatus src =
mremainingRef
idxhdr
nextBodyFlush
`UnliftIO.catchAny` \e -> do
`catch` \e -> do
settingsOnException settings (Just req) e
-- Don't throw the error again to prevent calling settingsOnException twice.
return CloseConnection
Expand All @@ -166,8 +165,8 @@ http1server settings ii conn transport app addr th istatus src =
-- and ignore. See: https://github.com/yesodweb/wai/issues/618

case keepAlive of
ReuseConnection -> loop SubsequentRequest
CloseConnection -> return ()
ReuseConnection -> loop SubsequentRequest
CloseConnection -> return ()

data ReuseConnection = ReuseConnection | CloseConnection

Expand All @@ -192,7 +191,7 @@ processRequest settings ii conn app th istatus src req mremainingRef idxhdr next
-- creating the request, we need to make sure that we don't get
-- an async exception before calling the ResponseSource.
keepAliveRef <- newIORef $ error "keepAliveRef not filled"
r <- UnliftIO.tryAny $ app req $ \res -> do
r <- try $ app req $ \res -> do
T.resume th
-- FIXME consider forcing evaluation of the res here to
-- send more meaningful error messages to the user.
Expand Down Expand Up @@ -226,27 +225,27 @@ processRequest settings ii conn app th istatus src req mremainingRef idxhdr next
then -- If there is an unknown or large amount of data to still be read
-- from the request body, simple drop this connection instead of
-- reading it all in to satisfy a keep-alive request.
case settingsMaximumBodyFlush settings of
Nothing -> do
flushEntireBody nextBodyFlush
T.resume th
return ReuseConnection
Just maxToRead -> do
let tryKeepAlive = do
-- flush the rest of the request body
isComplete <- flushBody nextBodyFlush maxToRead
if isComplete
then do
T.resume th
return ReuseConnection
else return CloseConnection
case mremainingRef of
Just ref -> do
remaining <- readIORef ref
if remaining <= maxToRead
then tryKeepAlive
else return CloseConnection
Nothing -> tryKeepAlive
case settingsMaximumBodyFlush settings of
Nothing -> do
flushEntireBody nextBodyFlush
T.resume th
return ReuseConnection
Just maxToRead -> do
let tryKeepAlive = do
-- flush the rest of the request body
isComplete <- flushBody nextBodyFlush maxToRead
if isComplete
then do
T.resume th
return ReuseConnection
else return CloseConnection
case mremainingRef of
Just ref -> do
remaining <- readIORef ref
if remaining <= maxToRead
then tryKeepAlive
else return CloseConnection
Nothing -> tryKeepAlive
else return CloseConnection

sendErrorResponse
Expand Down
15 changes: 10 additions & 5 deletions warp/Network/Wai/Handler/Warp/HTTP2.hs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module Network.Wai.Handler.Warp.HTTP2 (
http2server,
) where

import qualified Control.Exception as E
import qualified Data.ByteString as BS
import Data.IORef (readIORef)
import qualified Data.IORef as I
Expand All @@ -20,7 +21,6 @@ import Network.Socket.BufferPool
import Network.Wai
import Network.Wai.Internal (ResponseReceived (..))
import qualified System.TimeManager as T
import qualified UnliftIO

import Network.Wai.Handler.Warp.HTTP2.File
import Network.Wai.Handler.Warp.HTTP2.PushPromise
Expand Down Expand Up @@ -93,7 +93,7 @@ http2server label settings ii transport addr app h2req0 aux0 response = do
labelThread tid (label ++ " http2server " ++ show addr)
req <- toWAIRequest h2req0 aux0
ref <- I.newIORef Nothing
eResponseReceived <- UnliftIO.tryAny $ app req $ \rsp -> do
eResponseReceived <- E.try $ app req $ \rsp -> do
(h2rsp, st, hasBody) <- fromResponse settings ii req rsp
pps <- if hasBody then fromPushPromises ii req else return []
I.writeIORef ref $ Just (h2rsp, pps, st)
Expand All @@ -105,7 +105,12 @@ http2server label settings ii transport addr app h2req0 aux0 response = do
let msiz = fromIntegral <$> H2.responseBodySize h2rsp
logResponse req st msiz
mapM_ (logPushPromise req) pps
Left e -> do
Left e
-- killed by the local worker manager
| Just E.ThreadKilled <- E.fromException e -> return ()
-- killed by the local timeout manager
| Just T.TimeoutThread <- E.fromException e -> return ()
| otherwise -> do
S.settingsOnException settings (Just req) e
let ersp = S.settingsOnExceptionResponse settings e
st = responseStatus ersp
Expand Down Expand Up @@ -135,7 +140,7 @@ http2server label settings ii transport addr app h2req0 aux0 response = do
wrappedRecvN
:: T.Handle -> Int -> (BufSize -> IO ByteString) -> (BufSize -> IO ByteString)
wrappedRecvN th slowlorisSize readN bufsize = do
bs <- UnliftIO.handleAny handler $ readN bufsize
bs <- E.handle handler $ readN bufsize
-- TODO: think about the slowloris protection in HTTP2: current code
-- might open a slow-loris attack vector. Rather than timing we should
-- consider limiting the per-client connections assuming that in HTTP2
Expand All @@ -146,7 +151,7 @@ wrappedRecvN th slowlorisSize readN bufsize = do
T.tickle th
return bs
where
handler :: UnliftIO.SomeException -> IO ByteString
handler :: E.SomeException -> IO ByteString
handler _ = return ""

-- connClose must not be called here since Run:fork calls it
Expand Down
6 changes: 3 additions & 3 deletions warp/Network/Wai/Handler/Warp/HTTP2/PushPromise.hs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@

module Network.Wai.Handler.Warp.HTTP2.PushPromise where

import qualified Control.Exception as E
import qualified Network.HTTP.Types as H
import qualified Network.HTTP2.Server as H2
import qualified UnliftIO

import Network.Wai
import Network.Wai.Handler.Warp.FileInfoCache
Expand All @@ -22,9 +22,9 @@ fromPushPromises ii req = do

fromPushPromise :: InternalInfo -> PushPromise -> IO (Maybe H2.PushPromise)
fromPushPromise ii (PushPromise path file rsphdr w) = do
efinfo <- UnliftIO.tryIO $ getFileInfo ii file
efinfo <- E.try $ getFileInfo ii file
case efinfo of
Left (_ex :: UnliftIO.IOException) -> return Nothing
Left (_ex :: E.IOException) -> return Nothing
Right finfo -> do
let !siz = fromIntegral $ fileInfoSize finfo
!fileSpec = H2.FileSpec file 0 siz
Expand Down
Loading