warp: let the thread manager own the connections (#1126)

[kazu-yamamoto]

Fixes #1126 on master. #1127 is the same bug fixed for a 3.4 release, by other means.

Warp forks a thread per connection with settingsFork, and nothing else owns those threads: they are not children of the thread running runSettings in any sense the runtime knows about. An asynchronous exception to that thread handed the caller its exception back and left the applications in flight running.

Until 3.4.10 something did own them, by accident. Warp brackets a time manager with stopManager, which used to run every registered timeout action, and each connection was registered with withHandleKillThread. stopManager became a no-op in time-manager 0.3.0, and 3.4.10 is the version whose bound first allowed that time-manager. Warp's own code did not change between 3.4.9 and 3.4.10, which is why the reporter's bisection landed there.

System.ThreadManager is what owns threads here and in the http2 library, and stopAfter is what kills them. It did not fit as it was, for one reason: it registers a thread from inside the thread, and warp counts a connection from the accept loop, before the thread that will serve it has been scheduled. Counting later would let a graceful shutdown walk past a connection it had just accepted -- there is a test for that, waits for a connection accepted just before it stopped accepting. So the manager can be told to keep a place for a thread about to be forked (reserveManaged), and the thread takes that place over when it starts (takeReservation). countManaged counts both.

That also means one thing counts the connections and owns them, so warp's Counter is now a wrapper over the manager. getOpenConnectionCounter and getCount are unchanged.

settingsFork is still what forks, which forkManagedUnmask would not have allowed -- it forks with forkIOWithUnmask itself. The reporter's workaround sets settingsFork, so ignoring it was not an option.

Needs time-manager 0.4.1, in this PR. The second commit corrects forkManaged's haddock, which said the thread is registered before it starts.

The new test fails without the stopAfter.

[kazu-yamamoto]

@Vlix http2 and quic also wait for this PR to be merged.

[kazu-yamamoto]

I tried the branch against two cases and one of them does not hold.

stopAfter does not reach a reservation that has not been taken

stopAfterWithResult empties var and kills what was in it. It does not
look at rvar, so a thread whose place is still being kept is not killed,
and when it does start it registers itself into the now-empty map and runs
on with nobody owning it.

Against System.ThreadManager alone:

import Control.Concurrent
import Control.Concurrent.STM
import System.ThreadManager
import qualified System.TimeManager as T
import System.Timeout (timeout)

main :: IO ()
main = do
    tm <- newThreadManager T.defaultManager
    rsv <- reserveManaged tm
    survived <- newEmptyMVar
    -- Stands in for a thread settingsFork has made but not yet scheduled.
    _ <- forkIO $ do
        threadDelay 300000
        takeReservation tm rsv $ threadDelay 300000 >> putMVar survived ()
    atomically (countManaged tm) >>= \n -> putStrLn $ "before: " ++ show n
    stopAfter tm (return ()) (const (return ()))
    atomically (countManaged tm) >>= \n -> putStrLn $ "after:  " ++ show n
    r <- timeout 2000000 (takeMVar survived)
    putStrLn $ "ran to the end anyway: " ++ show (r == Just ())

before: 1
after:  1
ran to the end anyway: True

(ghc -threaded -i<path>/time-manager -XStrict -XStrictData.)

Warp reaches it. Added to GracefulShutdownSpec, this fails on the branch:

    it "kills a connection forked but not yet scheduled" $ do
        servingNow <- newEmptyMVar
        let app _ respond = do
                putMVar servingNow ()
                threadDelay 10_000_000
                respond $ responseLBS status200 [] ""
            set =
                setFork
                    (\io -> void $ forkIOWithUnmask $ \unmask -> threadDelay 300_000 >> io unmask)
                    defaultSettings
        bracket openFreePort (close . snd) $ \(testPort, sock) -> do
            server <- forkIO $ runSettingsSocket set sock app
            client <- openConnection testPort
            sendAll client "GET / HTTP/1.1\r\nHost: localhost\r\n\r\n"
            threadDelay 100_000
            killThread server
            timeout 2_000_000 (takeMVar servingNow) `shouldReturn` Nothing
            close client

expected: Nothing
 but got: Just ()

The application began serving after the server had been killed, which is
#1126 again. A settingsFork that does not schedule at once is what the
reporter's workaround sets, so this is not a contrived path.

With warp's own fork the window is one scheduling quantum. Having the
application record whether it started before or after the kill, 40 attempts
per delay: 2-10 of 40 at a 0us delay (it varies between runs), 0 of 40 at
50us and above. Read the first figure as an upper bound -- some of those
may have started between setting the flag and throwTo arriving. The
deterministic case above is the one I would trust.

A thought on the fix: having stopAfter wait for every reservation to be
taken would hang on a reservation that is never taken, so marking the
manager so that a thread registering after the stop is killed on arrival
looks better.

A reservation that is never taken

At the API level it never clears, so the manager never reaches zero:

    tm <- newThreadManager T.defaultManager
    _ <- reserveManaged tm
    timeout 500000 (waitUntilAllGone tm) >>= print   -- Nothing

Warp is not hurt by it, which I checked rather than assumed: with a
settingsFork that throws, the exception leaves acceptLoop, stopAfter
catches and rethrows it, gracefulShutdown is skipped, and the server ends
with the exception rather than hanging. So this is only about how easy the
API is to hold wrongly -- perhaps worth a line in reserveManaged's
haddock, since reserve and the fork that takes it are two statements with
nothing tying them together.

Everything else in warp:test:spec's graceful shutdown group passes on the
branch. Building it needed --allow-newer=http2:time-manager, since
http2 on Hackage still has <0.4.

[kazu-yamamoto]

@Vlix Apologies for the noise. The verification using Claude Code is complete, and it is safe to use from http2 and quic. Now, it’s time for a human review.