diff --git a/CHANGELOG.md b/CHANGELOG.md --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +# v1.0.1.6 + +## Fixes + +- Fixed a mid-handshake server rejection (e.g. "sorry, too many clients already") escaping as an uncaught `IOException` instead of coming back as a classified `ConnectionBad` (#8). `establish` only wrapped the initial TCP connect in an exception handler; the handshake read that follows it is now caught too and routed through the same `tcpFailureMessage`/`unixSocketFailureMessage` wrapper `failWith` uses, with libpq's own wording for the EOF case. `tcpFailureMessage` also stopped adding a redundant "(ip)" parenthetical when the resolved peer IP is identical to the given host. Caught by the `pqi-conformance` spec added in `nikita-volkov/pqi-conformance@92f5205`. + +- Fixed a pipelined command's result getting misattributed to a later, unrelated command after a prior pipeline aborted on a server error (#9). A pipelined command that sends a `Parse` (`sendQueryParams`/`sendPrepare`) records a FIFO entry so the eventual `ParseComplete` can be charged to the right command; that entry was only ever popped by a `ParseComplete` actually arriving. A command whose `Parse` itself fails - a syntax error, or being silently discarded by the server after a pipeline abort - never gets a `ParseComplete`, so its entry was leaked. A `sendPrepare` leaks a `True` entry, and once a later, unrelated command's genuine `ParseComplete` popped that stale `True` instead of its own, that command terminated immediately as `CommandOk` instead of collecting its real result, observed as a `SELECT` returning `CommandOk` instead of `TuplesOk`. Every pipelined command now pops its FIFO entry exactly once, whether via its own `ParseComplete` or, failing that, at its own terminal message (including the synthetic result generated for a command discarded after an abort). Caught by the differential coverage added in `pqi-conformance` 1.0.5.1. + # v1.0.1.5 ## Fixes diff --git a/pqi-native.cabal b/pqi-native.cabal --- a/pqi-native.cabal +++ b/pqi-native.cabal @@ -1,6 +1,6 @@ cabal-version: 3.0 name: pqi-native -version: 1.0.1.5 +version: 1.0.1.6 category: Database, PostgreSQL synopsis: Native (pure-Haskell) adapter for pqi description: @@ -158,5 +158,5 @@ build-depends: base >=4.11 && <5, hspec >=2.11 && <2.12, - pqi-conformance ^>=1.0.5, + pqi-conformance ^>=1.0.6, pqi-native, diff --git a/src/library/Pqi/Native/Connection.hs b/src/library/Pqi/Native/Connection.hs --- a/src/library/Pqi/Native/Connection.hs +++ b/src/library/Pqi/Native/Connection.hs @@ -32,6 +32,7 @@ import Pqi.Native.Types (formatErrorFields) import qualified PtrPoker.Write as Poker import System.Environment (lookupEnv) +import System.IO.Error (isEOFError) #if defined(mingw32_HOST_OS) import System.Win32.Info.Computer (getUserName) #else @@ -280,18 +281,31 @@ pipelineSeparatorPending :: IORef Bool, pendingSyncs :: IORef Int, pendingCommands :: IORef Int, - -- | FIFO, one entry per in-flight pipeline command that sends a @Parse@ - -- ('sendQueryParams' or 'sendPrepare'), recording whether that command's - -- @ParseComplete@ is itself the terminal result. 'sendPrepare' pushes - -- @True@ (its @ParseComplete@ ends the command as 'CommandOk'); - -- 'sendQueryParams' pushes @False@ (its @ParseComplete@ must fold into the - -- command's accumulating result, like every other extended-protocol - -- message, and never terminate it early). Popping the head at the right - -- @ParseComplete@ keeps the origins in order even when several commands are - -- pipelined together - a plain counter cannot, which is what let a - -- pipelined 'sendPrepare' steal the 'ParseComplete' of an earlier - -- 'sendQueryParams' and shift every later result by one. - pendingParseOrigins :: IORef (Seq.Seq Bool), + -- | FIFO, one entry per in-flight pipelined command, pushed by every + -- 'sendAsync' call while in pipeline mode and popped exactly once by + -- whichever message ends that command's processing. For a command that + -- sends a @Parse@ ('sendQueryParams' or 'sendPrepare') the entry records + -- whether that command's @ParseComplete@ is itself the terminal result: + -- 'sendPrepare' pushes @Just True@ (its @ParseComplete@ ends the command + -- as 'CommandOk'); 'sendQueryParams' pushes @Just False@ (its + -- @ParseComplete@ must fold into the command's accumulating result, like + -- every other extended-protocol message, and never terminate it early). + -- A command with no @Parse@ step (e.g. 'sendQueryPrepared') pushes + -- @Nothing@, popped (and discarded) at its own terminal message. Popping + -- the head at the right message keeps the origins in order even when + -- several commands are pipelined together - a plain counter cannot, + -- which is what let a pipelined 'sendPrepare' steal the 'ParseComplete' + -- of an earlier 'sendQueryParams' and shift every later result by one. + -- + -- Every command pushes exactly one entry and must pop exactly one, even + -- when it fails before reaching @ParseComplete@ (e.g. a syntax error, or + -- being discarded after a pipeline abort): leaving that entry unpopped + -- would let a later, unrelated command's @ParseComplete@ pop it instead, + -- misattributing that later command's origin and - when the leaked entry + -- happens to be @Just True@ - making it terminate early as 'CommandOk' + -- instead of collecting its actual result (e.g. 'TuplesOk' for a + -- @SELECT@). + pendingParseOrigins :: IORef (Seq.Seq (Maybe Bool)), errorVerbosity :: IORef Verbosity, -- | The SQL text of the most recently sent query (set by sendQuery / -- sendQueryParams). Used when formatting error messages for async results @@ -353,6 +367,25 @@ <> "')" | otherwise = "could not connect to server: " <> ByteString.Char8.pack (show err) +-- | Format a handshake-time 'IOException' - e.g. the server closing the +-- socket mid-rejection, as when shedding load with \"sorry, too many clients +-- already\": the rejection is sent but the socket closes before 'handshake' +-- finishes reading it. Routed through the same 'unixSocketFailureMessage'\/ +-- 'tcpFailureMessage' wrapper 'failWith' uses for a rejected 'ErrorResponse', +-- so a failure that interrupts the handshake reads exactly like any other +-- classified rejection instead of escaping 'establish' as an uncaught +-- exception. An EOF (the frame never completing) gets libpq's own wording for +-- it; any other handshake-time I\/O error falls back to its 'show'n form. +handshakeFailureMessage :: Connection -> ConnInfo -> IOException -> IO ByteString +handshakeFailureMessage connection connInfo err = do + let fmtFields + | isEOFError err = + "server closed the connection unexpectedly\n\tThis probably means the server terminated abnormally\n\tbefore or while processing the request.\n" + | otherwise = ByteString.Char8.pack (show err) + if Transport.isUnixSocketHost (host connInfo) + then pure (unixSocketFailureMessage connInfo fmtFields) + else tcpFailureMessage connection connInfo fmtFields + -- | The handshake-failure message ('failWith', inside 'handshake') for a -- Unix-domain socket connection: names the socket path rather than a -- host\/port pair, matching libpq's phrasing. @@ -364,20 +397,30 @@ <> fmtFields -- | The handshake-failure message for a TCP connection: includes the --- resolved peer IP when available (it may not be, e.g. if the socket has --- already been torn down), matching libpq's phrasing. +-- resolved peer IP when available and distinct from the given host (it may +-- not be resolvable at all, e.g. if the socket has already been torn down; +-- and libpq omits the parenthetical entirely when the host was already the +-- literal numeric address, rather than a name that resolved to it), matching +-- libpq's phrasing. tcpFailureMessage :: Connection -> ConnInfo -> ByteString -> IO ByteString tcpFailureMessage connection connInfo fmtFields = do transport <- readIORef (transport connection) mIp <- catch (Just <$> Transport.peerIp transport) (\(_ :: SomeException) -> pure Nothing) pure $ case mIp of - Nothing -> fmtFields - Just ip -> + Just ip + | ip /= host connInfo -> + "connection to server at \"" + <> host connInfo + <> "\" (" + <> ip + <> "), port " + <> ByteString.Char8.pack (show (port connInfo)) + <> " failed: " + <> fmtFields + _ -> "connection to server at \"" <> host connInfo - <> "\" (" - <> ip - <> "), port " + <> "\", port " <> ByteString.Char8.pack (show (port connInfo)) <> " failed: " <> fmtFields @@ -413,7 +456,10 @@ Right transport -> do connection <- newConnection False transport info sendMessage connection (startupMessage (startupParams info)) - handshake connection + handshakeResult <- try @IOException (handshake connection) + case handshakeResult of + Left err -> setError connection =<< handshakeFailureMessage connection info err + Right () -> pure () pure connection -- | A \"null\" sentinel connection (the analogue of @PQnewNullConnection@): no diff --git a/src/library/Pqi/Native/Query.hs b/src/library/Pqi/Native/Query.hs --- a/src/library/Pqi/Native/Query.hs +++ b/src/library/Pqi/Native/Query.hs @@ -144,7 +144,7 @@ -- unnamed @Parse@, whose @ParseComplete@ must fold into this command's own -- result (like 'sendQueryPrepared'). Record its origin so it is not mistaken -- for the terminal @ParseComplete@ of a pipelined 'sendPrepare'. - when (ok && pipeline) $ modifyIORef' (pendingParseOrigins connection) (Seq.|> False) + when (ok && pipeline) $ modifyIORef' (pendingParseOrigins connection) (Seq.|> Just False) pure ok sendPrepare :: Connection -> ByteString -> ByteString -> Maybe [Word32] -> IO Bool @@ -157,15 +157,24 @@ $ if pipeline then parseMessage name sql (fromMaybe [] parameterTypes) else prepareWrite name sql parameterTypes - when (ok && pipeline) $ modifyIORef' (pendingParseOrigins connection) (Seq.|> True) + when (ok && pipeline) $ modifyIORef' (pendingParseOrigins connection) (Seq.|> Just True) pure ok +-- | Push a @Nothing@ origin for a pipelined command that sends no @Parse@ +-- (so has no @ParseComplete@ to fold or terminate on): it still occupies one +-- slot in the FIFO, popped and discarded at its own terminal message. +sendAsyncNoOrigin :: Connection -> Bool -> ByteString -> Poker.Write -> IO Bool +sendAsyncNoOrigin connection pipeline sql write = do + ok <- sendAsync connection sql write + when (ok && pipeline) $ modifyIORef' (pendingParseOrigins connection) (Seq.|> Nothing) + pure ok + sendQueryPrepared :: Connection -> ByteString -> [Maybe (ByteString, Format)] -> Format -> IO Bool sendQueryPrepared connection name params resultFormat | tooManyParams params = pure False | otherwise = do pipeline <- inPipeline connection - sendAsync connection "" + sendAsyncNoOrigin connection pipeline "" $ if pipeline then asyncPreparedWrite name params resultFormat else preparedWrite name params resultFormat @@ -173,7 +182,7 @@ sendDescribePrepared :: Connection -> ByteString -> IO Bool sendDescribePrepared connection name = do pipeline <- inPipeline connection - sendAsync connection "" + sendAsyncNoOrigin connection pipeline "" $ if pipeline then describeStatementMessage name else describeStatementMessage name <> syncMessage @@ -181,7 +190,7 @@ sendDescribePortal :: Connection -> ByteString -> IO Bool sendDescribePortal connection name = do pipeline <- inPipeline connection - sendAsync connection "" + sendAsyncNoOrigin connection pipeline "" $ if pipeline then describePortalMessage name else describePortalMessage name <> syncMessage @@ -232,6 +241,14 @@ modifyIORef' (pendingCommands connection) (subtract 1) writeIORef (pipelineSeparatorPending connection) True + -- Pop and discard this command's 'pendingParseOrigins' entry if it hasn't + -- already been consumed by its own 'ParseComplete' (see the field's + -- Haddock on 'Connection'). Called at every terminal message besides a + -- 'ParseComplete' that itself terminates the command. + popOriginIfPending pipeStatus builder = + when (pipeStatus /= PipelineOff && not (accOriginPopped builder)) + $ void (popPendingParseOrigin connection) + go singleRow builder = do pipeStatus <- readIORef (pipelineStatus connection) -- In aborted pipeline mode, if the server has already sent nothing for @@ -241,6 +258,11 @@ pending <- readIORef (pendingCommands connection) if pipeStatus == PipelineAborted && pending > 0 then do + -- This command is being synthesized without ever reading a + -- message for it (the server sends nothing for a discarded + -- command), so its origin - if it pushed one - is necessarily + -- still unpopped. + popOriginIfPending pipeStatus builder modifyIORef' (pendingCommands connection) (subtract 1) writeIORef (pipelineSeparatorPending connection) True pure (Just (NativeResult PipelineAbort [] [] Nothing Map.empty [] "")) @@ -273,10 +295,11 @@ then popPendingParseOrigin connection else pure Nothing case origin of - Just True -> do + Just (Just True) -> do finishCommand pipeStatus pure (Just (NativeResult CommandOk [] [] Nothing Map.empty [] "")) - _ -> go singleRow builder {accHadResponse = True} + Just _ -> go singleRow builder {accHadResponse = True, accOriginPopped = True} + Nothing -> go singleRow builder {accHadResponse = True} BindComplete -> go singleRow builder {accHadResponse = True} CloseComplete -> @@ -290,10 +313,12 @@ pure (Just (NativeResult TuplesOk (accFields builder) [] (Just tag) Map.empty [] "")) else do writeIORef (lastError connection) (Just "") + popOriginIfPending pipeStatus builder finishCommand pipeStatus pure (Just (commandResult builder (Just tag))) EmptyQueryResponse -> do writeIORef (lastError connection) (Just "") + popOriginIfPending pipeStatus builder finishCommand pipeStatus pure (Just (NativeResult EmptyQuery [] [] Nothing Map.empty [] "")) ErrorResponse fs -> do @@ -302,10 +327,12 @@ PipelineAborted -> do -- Should not normally happen (server discards commands in abort -- mode) but handle defensively. + popOriginIfPending pipeStatus builder finishCommand pipeStatus pure (Just (NativeResult PipelineAbort [] [] Nothing Map.empty [] "")) PipelineOn -> do writeIORef (pipelineStatus connection) PipelineAborted + popOriginIfPending pipeStatus builder finishCommand PipelineOn pure (Just (NativeResult FatalError [] [] Nothing errMap [] "")) PipelineOff -> do @@ -335,12 +362,13 @@ pure (Just (NativeResult PipelineSync [] [] Nothing Map.empty [] "")) _ -> go singleRow builder --- | Pop the origin of the next in-flight @ParseComplete@ in pipeline mode: --- @True@ if it is the terminal @ParseComplete@ of a 'sendPrepare' (to be --- materialized as 'CommandOk'), @False@ if it belongs to a 'sendQueryParams' --- and must fold into that command's accumulating result. @Nothing@ when no --- @Parse@ is pending (defensive: the @ParseComplete@ is then folded). -popPendingParseOrigin :: Connection -> IO (Maybe Bool) +-- | Pop the next in-flight pipelined command's origin: @Just (Just True)@ if +-- it is the terminal @ParseComplete@ of a 'sendPrepare' (to be materialized +-- as 'CommandOk'), @Just (Just False)@ if it belongs to a 'sendQueryParams' +-- and must fold into that command's accumulating result, @Just Nothing@ for +-- a command with no @Parse@ step. The outer @Nothing@ means the FIFO is +-- empty (defensive: nothing is popped and the caller folds). +popPendingParseOrigin :: Connection -> IO (Maybe (Maybe Bool)) popPendingParseOrigin connection = atomicModifyIORef' (pendingParseOrigins connection) @@ -402,11 +430,14 @@ accRevRows :: [[Maybe ByteString]], accParamOids :: [Word32], accSawRowDescription :: Bool, - accHadResponse :: Bool + accHadResponse :: Bool, + -- | Whether this command's 'pendingParseOrigins' entry has already been + -- popped (via its own 'ParseComplete'). See 'popOriginIfPending'. + accOriginPopped :: Bool } emptyBuilder :: Builder -emptyBuilder = Builder [] [] [] False False +emptyBuilder = Builder [] [] [] False False False -- | Collect the (possibly several) results of a simple query, up to -- @ReadyForQuery@. The last is what @PQexec@ returns.