diff --git a/ChangeLog.md b/ChangeLog.md --- a/ChangeLog.md +++ b/ChangeLog.md @@ -1,5 +1,58 @@ # ChangeLog +## 0.3.15 + +A server could not be stopped without closing a socket under it, and its +peers were told nothing when it was. + +* Hand the caller the action that stops the server, through + `scInstallShutdownHandler`, the way warp hands out the action that + stops a warp. `stop` reaches a server through one of its connections, + and a server is at its emptiest when someone wants it to stop: one that + never took a connection, or has finished the ones it had, could not be + stopped at all. What a caller was left with was closing the socket out + from under the dispatcher waiting on it, which ends the server by + making it fail and closes a socket that in `runWithSockets` is not the + server's to close. The dispatchers already wait for a datagram and for + this at once, so they see it where they wait and end there: no socket + is closed and nothing is raised. Which matters beyond being tidy -- + waking a thread out of a wait by closing the file descriptor under it + is what `closeFdWith` is for, and the IO manager that provides it is + not the only one there will be. + [#159](https://github.com/kazu-yamamoto/quic/pull/159) + +* Tell the peers when the server stops. A server that stopped closed its + sockets and that was all they ever learned of it: each connection went + quiet and stayed quiet until the peer's idle timeout expired, half a + minute later, on a connection that was never going to answer again. + The connections are now ended through the same path that ends a + connection for any other reason, while the sockets are still open, so + each peer is sent a CONNECTION_CLOSE and can open a new connection at + once. It is an application close and `scCloseReason` says which: a + transport CONNECTION_CLOSE carrying NO_ERROR is what a connection whose + application has finished normally sends, and a client makes an + exception of it in a 1-RTT packet so that a server finishing is not an + error. A server that is going away is saying something else, and the + application protocol is where it is said -- HTTP/3 has H3_NO_ERROR for + it. + [#159](https://github.com/kazu-yamamoto/quic/pull/159) + +* Send the first CONNECTION_CLOSE before leaving the connection. The + frame was encoded where the connection ends and the sending left to a + closer thread that nothing waited for, so the connection ended before + it had gone anywhere -- and a server that stops lets go of its sockets + as soon as its connections have ended, so the send then failed on a + closed socket and the peer heard nothing at all. + [#159](https://github.com/kazu-yamamoto/quic/pull/159) + +* Set `SO_REUSEPORT` on macOS and the BSDs. Replacing a server means + starting its successor while the old process still has the UDP port, + and `SO_REUSEADDR` alone does not allow that there: the second bind is + refused with "Address already in use" and the successor cannot start. + It is not set on Linux, where it would load balance one server's + datagrams across the two processes by a hash of the four-tuple. + [#159](https://github.com/kazu-yamamoto/quic/pull/159) + ## 0.3.14 A buffer overrun on many streams at once, 0-RTT sent against no limit at diff --git a/Network/QUIC/Closer.hs b/Network/QUIC/Closer.hs --- a/Network/QUIC/Closer.hs +++ b/Network/QUIC/Closer.hs @@ -113,6 +113,13 @@ -- hook let hook = onCloseCompleted $ connHooks conn pto <- getPTO ldcc + -- The first one goes out here, in the thread that is ending the + -- connection, rather than in the closer below: a server that is + -- stopping lets go of its sockets as soon as its connections have + -- ended, and a CONNECTION_CLOSE still waiting its turn on a thread + -- of its own misses them. The closer repeats it for a peer that + -- did not hear it, which is what it is for. + send void $ forkFinally (closer conn pto send recv hook) $ \e -> do case e of Left e' -> connDebugLog conn $ "closure' " <> bhow e' diff --git a/Network/QUIC/Config.hs b/Network/QUIC/Config.hs --- a/Network/QUIC/Config.hs +++ b/Network/QUIC/Config.hs @@ -199,6 +199,23 @@ , scDebugLog :: Maybe FilePath , scTicketLifetime :: Int -- ^ A lifetime (in seconds) for TLS session ticket and QUIC token. + , scCloseReason :: (ApplicationProtocolError, ReasonPhrase) + -- ^ What the connections still open are told when the server is + -- stopped. + -- + -- An application close rather than a transport one, and deliberately: + -- a transport CONNECTION_CLOSE carrying NO_ERROR is what a connection + -- whose application has finished normally sends, and a client makes an + -- exception of it so that a server finishing is not an error. A server + -- that is going away is saying something else, and the application + -- protocol is where it is said: HTTP/3 has H3_NO_ERROR (0x100) for it. + , scInstallShutdownHandler :: IO () -> IO () + -- ^ Handed the action that stops this server, once, as the server + -- starts. Keeping it is what lets the caller stop a server that has no + -- connections to stop it through, and stopping one this way closes no + -- socket and raises nothing: the dispatchers see it where they wait for + -- a datagram and end, the connections are told the server is going, and + -- 'run' returns. The default does nothing with it. } -- | The default value for server configuration. @@ -229,4 +246,6 @@ , scSessionManager = noSessionManager , scDebugLog = Nothing , scTicketLifetime = 7200 + , scCloseReason = (ApplicationProtocolError 0, "server is closing") + , scInstallShutdownHandler = \_ -> return () } diff --git a/Network/QUIC/Server.hs b/Network/QUIC/Server.hs --- a/Network/QUIC/Server.hs +++ b/Network/QUIC/Server.hs @@ -20,6 +20,8 @@ -- , scParameters scCredentials, scSessionManager, + scCloseReason, + scInstallShutdownHandler, -- * Certificate clientCertificateChain, diff --git a/Network/QUIC/Server/Reader.hs b/Network/QUIC/Server/Reader.hs --- a/Network/QUIC/Server/Reader.hs +++ b/Network/QUIC/Server/Reader.hs @@ -6,6 +6,9 @@ newDispatch, clearDispatch, runDispatcher, + liveConnections, + withConnectionCount, + waitNoConnection, tokenMgr, genStatelessReset, @@ -20,6 +23,7 @@ import Control.Concurrent import Control.Concurrent.STM +import qualified Control.Monad.STM as STM import qualified Control.Exception as E import qualified Crypto.Token as CT import qualified Data.ByteString as BS @@ -55,6 +59,7 @@ , srcTable :: RecvQDict , genStatelessReset :: CID -> StatelessResetToken , statelessResetRate :: Rate + , connectionCount :: TVar Int } statelessResetLimit :: Int @@ -68,6 +73,7 @@ <*> newRecvQDict <*> makeGenStatelessReset <*> newRate + <*> newTVarIO 0 where conf = CT.defaultConfig @@ -97,6 +103,28 @@ unregisterConnectionDict :: IORef ConnectionDict -> CID -> IO () unregisterConnectionDict ref cid = atomicModifyIORef'' ref $ \(ConnectionDict tbl) -> ConnectionDict $ M.delete cid tbl + +-- | The connections the server still has, each of them once. +liveConnections :: Dispatch -> IO [Connection] +liveConnections Dispatch{..} = do + ConnectionDict tbl <- readIORef dstTable + -- A connection is in the table under each of the CIDs it answers to, so + -- the elements repeat. Its main thread is what it has one of. + return $ M.elems $ M.fromList [(mainThreadId conn, conn) | conn <- M.elems tbl] + +-- | Running a connection, counted while it runs. 'dstTable' cannot say +-- when the last of them is done: a connection leaves it a second after it +-- ends, and it is in there under several keys. +withConnectionCount :: Dispatch -> IO a -> IO a +withConnectionCount Dispatch{..} = E.bracket_ (bump 1) (bump (-1)) + where + bump n = atomically $ modifyTVar' connectionCount (+ n) + +-- | Waiting until no connection is running. +waitNoConnection :: Dispatch -> IO () +waitNoConnection Dispatch{..} = atomically $ do + n <- readTVar connectionCount + STM.check $ n == 0 ---------------------------------------------------------------- diff --git a/Network/QUIC/Server/Run.hs b/Network/QUIC/Server/Run.hs --- a/Network/QUIC/Server/Run.hs +++ b/Network/QUIC/Server/Run.hs @@ -13,6 +13,7 @@ import Control.Concurrent.STM import qualified Control.Exception as E import qualified Network.Socket as NS +import qualified System.Timeout as T import Network.QUIC.Closer import Network.QUIC.Common @@ -43,6 +44,7 @@ run conf server = do labelMe "QUIC run" stvar <- newTVarIO Running + installShutdownHandler conf stvar -- Outside handleLogUnit on purpose. If the addresses cannot be bound -- there is no server, and that is the caller's business: swallowing it -- returned from 'run' as if all were well, having never reached @@ -63,7 +65,11 @@ -- took, which for a list means each element frees itself. setup stvar = do dispatch <- newDispatch conf - let forkConn acc = void $ forkIO (runServer conf server dispatch stvar acc) + let forkConn acc = + void $ + forkIO $ + withConnectionCount dispatch $ + runServer conf server dispatch stvar acc flip E.onException (clearDispatch dispatch) $ do ssas <- openAll $ scAddresses conf tids <- @@ -73,6 +79,7 @@ teardown (dispatch, tids, ssas) = do clearDispatch dispatch mapM_ killThread tids + shutdownConnections conf dispatch mapM_ NS.close ssas openAll [] = return [] openAll (a : as) = @@ -85,6 +92,7 @@ runWithSockets ssas conf server = do labelMe "QUIC runWithSockets" stvar <- newTVarIO Running + installShutdownHandler conf stvar -- As in 'run'. E.bracket (setup stvar) teardown $ \(_, _) -> handleLogUnit debugLog $ do onServerReady $ scHooks conf @@ -97,13 +105,72 @@ -- manager and the dispatchers are ours to free. setup stvar = do dispatch <- newDispatch conf - let forkConn acc = void $ forkIO (runServer conf server dispatch stvar acc) + let forkConn acc = + void $ + forkIO $ + withConnectionCount dispatch $ + runServer conf server dispatch stvar acc flip E.onException (clearDispatch dispatch) $ do tids <- runAll dispatch conf stvar forkConn ssas return (dispatch, tids) teardown (dispatch, tids) = do clearDispatch dispatch mapM_ killThread tids + shutdownConnections conf dispatch + +-- | Handing the caller the action that stops this server. +-- +-- 'stop' is the same action, reached through a connection; a server with no +-- connections cannot be stopped that way, and a server is at its emptiest +-- when someone wants it to stop. +-- +-- Stopping closes no socket and raises nothing. The dispatchers wait for a +-- datagram and for this at once, so they see it where they wait and end +-- there; 'run' then ends the connections and returns, and the sockets are +-- the caller's to close. Which matters beyond being tidy: waking a thread +-- out of a wait by closing the file descriptor under it is what +-- 'closeFdWith' is for, and the IO manager that provides it is not the only +-- one there will be. +installShutdownHandler :: ServerConfig -> TVar ServerState -> IO () +installShutdownHandler conf stvar = + scInstallShutdownHandler conf $ atomically $ writeTVar stvar Stopped + +-- | Ending the connections the server still has, while the sockets are +-- still open. +-- +-- Killing the dispatchers above is what stops the server taking new +-- connections: nothing reads the sockets any more, so an Initial that +-- arrives now goes unanswered and is retried a PTO later -- by which time +-- the successor has the port and answers it instead. +-- +-- What killing them cannot do is say anything to the connections that are +-- already here. Left alone they find out when the sockets close under +-- them, which is too late to tell anyone, and each peer is left with a +-- connection that answers nothing until its idle timeout expires half a +-- minute later. So each of them is ended here, through the same path that +-- ends a connection for any other reason, and the peer is sent a +-- CONNECTION_CLOSE while there is still a socket to send it on. A peer +-- that hears it can open a new connection at once, and the successor is +-- there to answer. +-- +-- Hearing the peers is another matter: once the successor has the port, on +-- Linux it is the successor that receives. See Note [Binding the same port +-- twice]. Sending still works, which is what this relies on. +-- +-- Each is ended from a thread of its own, because 'throwTo' waits for the +-- exception to be taken and a connection whose application is slow to +-- unwind would hold up the rest. The wait that follows is what they are +-- all given, and it is bounded: a connection that will not end is not worth +-- the sockets. +shutdownConnections :: ServerConfig -> Dispatch -> IO () +shutdownConnections conf dispatch = do + conns <- liveConnections dispatch + unless (null conns) $ do + mapM_ (void . forkIO . shut) conns + void $ T.timeout 1000000 $ waitNoConnection dispatch + where + (err, reason) = scCloseReason conf + shut conn = abortConnection conn err reason -- | Running a dispatcher on each socket, killing the ones already running if -- a later one cannot be started. diff --git a/Network/QUIC/Socket.hs b/Network/QUIC/Socket.hs --- a/Network/QUIC/Socket.hs +++ b/Network/QUIC/Socket.hs @@ -1,3 +1,5 @@ +{-# LANGUAGE CPP #-} + module Network.QUIC.Socket ( serverSocket, clientSocket, @@ -27,9 +29,35 @@ where hints = defaultHints{addrSocketType = Datagram, addrFlags = [AI_ADDRCONFIG]} +-- Note [Binding the same port twice] +-- +-- A server is replaced by starting its successor while the old process still +-- has the port, so for a moment two processes have the same UDP port bound. +-- What makes that possible, and what happens while it lasts, differs between +-- the platforms. Both of these were measured, not assumed: +-- +-- Linux, SO_REUSEADDR: the second bind succeeds, and the socket bound last +-- receives everything from that moment on. The old process goes deaf while +-- its socket is still open. It can still send, which is what the +-- CONNECTION_CLOSE sweep in Network.QUIC.Server.Run relies on. +-- +-- macOS and the BSDs, SO_REUSEADDR alone: the second bind is refused with +-- "Address already in use" and the successor cannot start at all. With +-- SO_REUSEPORT the second bind succeeds, and the socket bound *first* keeps +-- receiving everything until it is closed, at which point the other takes +-- over with nothing lost in between. +-- +-- SO_REUSEPORT is deliberately not set on Linux, where it means something +-- else: the kernel load balances datagrams between the two sockets by a hash +-- of the four-tuple, which would split one server's packets across two +-- processes at random. + serverSocket :: (IP, PortNumber) -> IO Socket serverSocket ip = E.bracketOnError open close $ \s -> do setSocketOption s ReuseAddr 1 +#if defined(darwin_HOST_OS) || defined(freebsd_HOST_OS) || defined(netbsd_HOST_OS) || defined(openbsd_HOST_OS) + setSocketOption s ReusePort 1 +#endif withFdSocket s setCloseOnExecIfNeeded bind s sa return s diff --git a/quic.cabal b/quic.cabal --- a/quic.cabal +++ b/quic.cabal @@ -1,6 +1,6 @@ cabal-version: 2.0 name: quic -version: 0.3.14 +version: 0.3.15 license: BSD3 license-file: LICENSE maintainer: kazu@iij.ad.jp @@ -240,6 +240,7 @@ RecoverySpec ResetSpec SetupSpec + ShutdownSpec TLSSpec TokenSpec TransportError diff --git a/test/ShutdownSpec.hs b/test/ShutdownSpec.hs new file mode 100644 --- /dev/null +++ b/test/ShutdownSpec.hs @@ -0,0 +1,105 @@ +{-# LANGUAGE OverloadedStrings #-} +{-# LANGUAGE ScopedTypeVariables #-} + +-- | Stopping a server through the handler it hands out. +-- +-- The point of it is what it does not do. A server used to be stopped by +-- closing the socket under the dispatcher waiting on it, which takes an IO +-- manager that can wake a thread out of a wait by closing its file +-- descriptor, and tells the peers of every connection nothing at all. +module ShutdownSpec where + +import Control.Concurrent +import Control.Concurrent.Async +import qualified Control.Exception as E +import qualified Network.Socket as NS +import qualified System.Timeout as T +import Test.Hspec + +import Network.QUIC +import qualified Network.QUIC.Client as C +import Network.QUIC.Internal +import Network.QUIC.Server + +import Config + +spec :: Spec +spec = describe "the shutdown handler" $ do + it "ends a server that has no connection to end it through" $ + withServerSocket $ \sock -> do + (sc, ready, stopVar) <- shutdownServerConfig + withAsync (runWithSockets [sock] sc $ \_ -> return ()) $ \server -> do + takeMVar ready + stopNow <- takeMVar stopVar + stopNow + ended <- T.timeout 5000000 $ wait server + ended `shouldBe` Just () + -- The socket is the caller's, and still is: stopping the server + -- did not close it. + NS.getSocketName sock `shouldReturn` serverSockAddr + + it "tells a connected client before it goes" $ + withServerSocket $ \sock -> do + (sc, ready, stopVar) <- shutdownServerConfig + let forever' = threadDelay 30000000 + -- Said from the server's side of the connection, not the + -- client's: the server reaches its application once the + -- handshake is behind it and the connection is one it has, and + -- stopping before that is a race the client cannot see. + serving <- newEmptyMVar + withAsync (runWithSockets [sock] sc $ \_ -> putMVar serving () >> forever') $ + \server -> do + takeMVar ready + stopNow <- takeMVar stopVar + withAsync (C.run clientConfig (\_ -> forever')) $ \peer -> do + takeMVar serving + stopNow + -- The server first, so that a server that did not + -- stop is not reported as a client that was not + -- told. + ended <- T.timeout 5000000 $ wait server + ended `shouldBe` Just () + told <- T.timeout 5000000 $ waitCatch peer + told `shouldSatisfy` wasToldTheServerIsClosing + +-- | Whether the client ended because the server said so, rather than +-- because it waited out its idle timeout on a connection that had stopped +-- answering. +wasToldTheServerIsClosing :: Maybe (Either E.SomeException ()) -> Bool +wasToldTheServerIsClosing (Just (Left se)) = case E.fromException se of + Just (ApplicationProtocolErrorIsReceived _ reason) -> reason == serverIsClosing + _ -> False +wasToldTheServerIsClosing _ = False + +-- | What a stopping server says, which is 'scCloseReason's default. +serverIsClosing :: ReasonPhrase +serverIsClosing = "server is closing" + +-- A port of its own. Two UDP sockets may hold one port between them, so a +-- port another spec has not finished with is not a bind that fails, it is a +-- test that reads someone else's datagrams. +serverPort :: NS.PortNumber +serverPort = 15004 + +serverSockAddr :: NS.SockAddr +serverSockAddr = NS.SockAddrInet serverPort $ NS.tupleToHostAddress (127, 0, 0, 1) + +withServerSocket :: (NS.Socket -> IO a) -> IO a +withServerSocket = E.bracket (serverSocket ("127.0.0.1", serverPort)) NS.close + +clientConfig :: ClientConfig +clientConfig = testClientConfig{ccPortName = show serverPort} + +-- | A server that says when it is ready and hands out the action that stops +-- it. +shutdownServerConfig :: IO (ServerConfig, MVar (), MVar (IO ())) +shutdownServerConfig = do + sc0 <- makeTestServerConfig + ready <- newEmptyMVar + stopVar <- newEmptyMVar + let sc = + sc0 + { scHooks = (scHooks sc0){onServerReady = putMVar ready ()} + , scInstallShutdownHandler = putMVar stopVar + } + return (sc, ready, stopVar)