Skip to content

Commit 4fc4775

Browse files
mkleczeksteve-chavez
authored andcommitted
refactor: do not open a TCP connection in liveness check
This change introduces a new way to perform liveness check - instead of trying to open a TCP connection to main server, we check if main socket is listening and if main server accept loop thread is alive. Opening a TCP connection in liveness check was problematic because: * it used available file descriptors which might have been a problem under load * made liveness check unreliable when multiple PostgREST instances are available on the same port (eg. using SO_REUSEPORT)
1 parent 0bda2bc commit 4fc4775

2 files changed

Lines changed: 27 additions & 24 deletions

File tree

src/PostgREST/App.hs

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Some of its functionality includes:
1010
- Content Negotiation
1111
-}
1212
{-# LANGUAGE FlexibleContexts #-}
13+
{-# LANGUAGE LambdaCase #-}
1314
{-# LANGUAGE NamedFieldPuns #-}
1415
{-# LANGUAGE RecordWildCards #-}
1516
{-# LANGUAGE ScopedTypeVariables #-}
@@ -19,8 +20,9 @@ module PostgREST.App
1920
, run
2021
) where
2122

22-
23+
import GHC.Conc (ThreadStatus (..), threadStatus)
2324
import GHC.IO.Exception (IOErrorType (..))
25+
import GHC.Weak
2426
import System.IO.Error (ioeGetErrorType)
2527

2628
import Control.Monad.Except (liftEither)
@@ -71,14 +73,14 @@ import qualified Data.Text as T
7173
import qualified Network.HTTP.Types as HTTP
7274
import Network.HTTP.Types.Header (hVary)
7375
import qualified Network.Socket as NS
74-
import Network.Socket.ByteString (send)
7576
import PostgREST.Unix (createAndBindDomainSocket)
7677
import System.Posix.Types (FileMode)
7778

78-
import Protolude hiding (Handler)
79+
import Protolude hiding (Handler)
80+
import System.Directory (doesPathExist)
7981

80-
run :: AppState -> IO ()
81-
run appState = do
82+
run :: AppState -> Weak ThreadId -> IO ()
83+
run appState mainThreadIdRef = do
8284
conf <- AppState.getConfig appState
8385

8486
mainSocketRef <- newIORef Nothing
@@ -92,7 +94,7 @@ run appState = do
9294
ensureSocketClosed =<< readIORef mainSocketRef
9395
Unix.installSignalHandlers observer closeSockets (AppState.schemaCacheLoader appState) (AppState.readInDbConfig False appState)
9496

95-
Admin.runAdmin appState adminSocket (checkMainAppLive (readIORef mainSocketRef)) (serverSettings conf)
97+
Admin.runAdmin appState adminSocket (checkMainAppLive (readIORef mainSocketRef) mainThreadIdRef) (serverSettings conf)
9698

9799
Listener.runListener appState
98100

@@ -297,21 +299,21 @@ initAdminServerSocket AppConfig{..} =
297299
configAdminServerUnixSocket configAdminServerUnixSocketMode
298300
configAdminServerHost configAdminServerPort
299301

300-
checkMainAppLive :: IO (Maybe NS.Socket) -> IO Bool
301-
checkMainAppLive getMainSocket =
302-
getMainSocket >>= maybe (pure False) (fmap isRight . reachMainApp)
303-
304-
-- Try to connect to the main app socket
305-
-- Note that it doesn't even send a valid HTTP request, we just want to check that the main app is accepting connections
306-
reachMainApp :: NS.Socket -> IO (Either IOException ())
307-
reachMainApp appSock = do
308-
sockAddr <- NS.getSocketName appSock
309-
sock <- NS.socket (addrFamily sockAddr) NS.Stream NS.defaultProtocol
310-
try $ do
311-
NS.connect sock sockAddr
312-
NS.withSocketsDo $ bracket (pure sock) NS.close sendEmpty
302+
checkMainAppLive :: IO (Maybe NS.Socket) -> Weak ThreadId -> IO Bool
303+
checkMainAppLive getMainSocket mainThreadIdRef =
304+
handle (\(_ :: IOException) -> pure False) $
305+
checkMainThread <&&> checkSocket
313306
where
314-
sendEmpty sock = void $ send sock mempty
315-
addrFamily (NS.SockAddrInet _ _) = NS.AF_INET
316-
addrFamily (NS.SockAddrInet6 {}) = NS.AF_INET6
317-
addrFamily (NS.SockAddrUnix _) = NS.AF_UNIX
307+
checkSocket = getMainSocket >>=
308+
maybe (pure False)
309+
(NS.getSocketName >=> \case
310+
-- in case of unix socket, check if it still exists
311+
NS.SockAddrUnix fp -> doesPathExist fp
312+
_ -> pure True)
313+
checkMainThread = deRefWeak mainThreadIdRef >>=
314+
maybe (pure False)
315+
(fmap isRunning . threadStatus)
316+
isRunning = \case
317+
ThreadRunning -> True
318+
ThreadBlocked _ -> True
319+
_ -> False

src/PostgREST/CLI.hs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ runClientCommand conf CmdReady = Client.ready conf
4343
runAppCommand :: AppConfig -> RunCommand -> IO ()
4444
runAppCommand conf@AppConfig{..} runCmd = do
4545
mainThreadId <- myThreadId
46+
mainThreadIdRef <- mkWeakThreadId mainThreadId
4647
-- Per https://github.com/PostgREST/postgrest/issues/268, we want to
4748
-- explicitly close the connections to PostgreSQL on shutdown.
4849
-- 'AppState.destroy' takes care of that.
@@ -56,7 +57,7 @@ runAppCommand conf@AppConfig{..} runCmd = do
5657
CmdDumpSchema -> do
5758
when configDbConfig $ AppState.readInDbConfig True appState
5859
putStrLn =<< dumpSchema appState
59-
CmdRun -> App.run appState)
60+
CmdRun -> App.run appState mainThreadIdRef)
6061

6162
-- | Dump SchemaCache schema to JSON
6263
dumpSchema :: AppState -> IO LBS.ByteString

0 commit comments

Comments
 (0)