From 11d8da046cf8de984fd3785e708e091add27845d Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Fri, 15 Mar 2024 10:24:52 -0500 Subject: [PATCH] fix: incorrect /ready response on slow schema load --- CHANGELOG.md | 1 + src/PostgREST/Admin.hs | 2 +- src/PostgREST/AppState.hs | 12 ++++++++++++ test/io/postgrest.py | 8 ++++---- test/io/test_big_schema.py | 20 ++++++++++++++++++++ 5 files changed, 38 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51de9304c..55f308991 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #3160, Fix using select= query parameter for custom media type handlers - @wolfgangwalther - #3237, Dump media handlers and timezones with --dump-schema - @wolfgangwalther - #3323, #3324, Don't hide error on LISTEN channel failure - @steve-chavez + - #3330, Incorrect admin server `/ready` response on slow schema cache loads - @steve-chavez ### Deprecated diff --git a/src/PostgREST/Admin.hs b/src/PostgREST/Admin.hs index 1429816e1..70df961ff 100644 --- a/src/PostgREST/Admin.hs +++ b/src/PostgREST/Admin.hs @@ -38,7 +38,7 @@ runAdmin conf@AppConfig{configAdminServerPort} appState settings observer = admin :: AppState.AppState -> AppConfig -> (Observation -> IO ()) -> Wai.Application admin appState appConfig observer req respond = do isMainAppReachable <- isRight <$> reachMainApp (AppState.getSocketREST appState) - isSchemaCacheLoaded <- isJust <$> AppState.getSchemaCache appState + isSchemaCacheLoaded <- AppState.getSchemaCacheLoaded appState isConnectionUp <- if configDbChannelEnabled appConfig then AppState.getIsListenerOn appState diff --git a/src/PostgREST/AppState.hs b/src/PostgREST/AppState.hs index 4fd6d3a78..e05e777fb 100644 --- a/src/PostgREST/AppState.hs +++ b/src/PostgREST/AppState.hs @@ -16,6 +16,7 @@ module PostgREST.AppState , getJwtCache , getSocketREST , getSocketAdmin + , getSchemaCacheLoaded , init , initSockets , initWithPool @@ -85,6 +86,8 @@ data AppState = AppState , statePgVersion :: IORef PgVersion -- | No schema cache at the start. Will be filled in by the connectionWorker , stateSchemaCache :: IORef (Maybe SchemaCache) + -- | If schema cache is loaded + , stateSchemaCacheLoaded :: IORef Bool -- | starts the connection worker with a debounce , debouncedConnectionWorker :: IO () -- | Binary semaphore used to sync the listener(NOTIFY reload) with the connectionWorker. @@ -123,6 +126,7 @@ initWithPool (sock, adminSock) pool conf observer = do appState <- AppState pool <$> newIORef minimumPgVersion -- assume we're in a supported version when starting, this will be corrected on a later step <*> newIORef Nothing + <*> newIORef False <*> pure (pure ()) <*> newEmptyMVar <*> newIORef False @@ -283,6 +287,12 @@ getIsListenerOn = readIORef . stateIsListenerOn putIsListenerOn :: AppState -> Bool -> IO () putIsListenerOn = atomicWriteIORef . stateIsListenerOn +getSchemaCacheLoaded :: AppState -> IO Bool +getSchemaCacheLoaded = readIORef . stateSchemaCacheLoaded + +putSchemaCacheLoaded :: AppState -> Bool -> IO () +putSchemaCacheLoaded = atomicWriteIORef . stateSchemaCacheLoaded + -- | Schema cache status data SCacheStatus = SCLoaded @@ -305,6 +315,7 @@ loadSchemaCache appState observer = do Nothing -> do putSchemaCache appState Nothing observer $ SchemaCacheNormalErrorObs e + putSchemaCacheLoaded appState False return SCOnRetry Right sCache -> do @@ -312,6 +323,7 @@ loadSchemaCache appState observer = do observer $ SchemaCacheQueriedObs resultTime (t, _) <- timeItT $ observer $ SchemaCacheSummaryObs sCache observer $ SchemaCacheLoadedObs t + putSchemaCacheLoaded appState True return SCLoaded -- | Current database connection status data ConnectionStatus diff --git a/test/io/postgrest.py b/test/io/postgrest.py index 307c58ebb..6fd46613c 100644 --- a/test/io/postgrest.py +++ b/test/io/postgrest.py @@ -78,6 +78,7 @@ def run( port=None, host=None, wait_for_readiness=True, + wait_max_seconds=1, no_pool_connection_available=False, no_startup_stdout=True, ): @@ -118,7 +119,7 @@ def run( process.stdin.close() if wait_for_readiness: - wait_until_ready(adminurl + "/ready") + wait_until_ready(adminurl + "/ready", wait_max_seconds) if no_startup_stdout: process.stdout.read() @@ -176,12 +177,11 @@ def wait_until_exit(postgrest): raise PostgrestTimedOut() -def wait_until_ready(url): +def wait_until_ready(url, max_seconds): "Wait for the given HTTP endpoint to return a status of 200." session = requests_unixsocket.Session() - response = None - for _ in range(10): + for _ in range(max_seconds * 10): try: response = session.get(url, timeout=1) if response.status_code == 200: diff --git a/test/io/test_big_schema.py b/test/io/test_big_schema.py index 86bfab259..403232bda 100644 --- a/test/io/test_big_schema.py +++ b/test/io/test_big_schema.py @@ -7,6 +7,26 @@ from util import * from postgrest import * +def test_first_request_succeeds(defaultenv): + "the first request suceeds fast since the admin server readiness was checked before" + + env = { + **defaultenv, + "PGRST_DB_SCHEMAS": "apflora", + "PGRST_DB_POOL": "2", + "PGRST_DB_ANON_ROLE": "postgrest_test_anonymous", + "PGRST_SERVER_TIMING_ENABLED": "true", + } + + with run(env=env, wait_max_seconds=20) as postgrest: + response = postgrest.session.get("/tpopmassn?select=*,tpop(*)") + assert response.status_code == 200 + + server_timings = parse_server_timings_header(response.headers["Server-Timing"]) + plan_dur = server_timings["plan"] + assert plan_dur < 2.0 + + def test_requests_wait_for_schema_cache_to_be_loaded(defaultenv): "requests that use the schema cache (e.g. resource embedding) wait for schema cache to be loaded"