Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slow or stuck stream consumer can cause memory to blow up #62

Open
akshaymankar opened this issue Mar 30, 2023 · 7 comments
Open

Slow or stuck stream consumer can cause memory to blow up #62

akshaymankar opened this issue Mar 30, 2023 · 7 comments
Assignees

Comments

@akshaymankar
Copy link

Here is an example with a stuck stream consumer:

{-# LANGUAGE OverloadedStrings #-}

module Main where

import Control.Concurrent
import Control.Concurrent.Async
import Control.Exception
import Control.Monad
import qualified Data.ByteString as BS
import qualified Data.ByteString.Builder as Builder
import qualified Data.ByteString.Lazy as LBS
import Data.Streaming.Network
import Network.HTTP.Types
import qualified Network.HTTP2.Client as Client
import qualified Network.HTTP2.Server as Server
import Network.Socket

testServer :: Server.Request -> Server.Aux -> (Server.Response -> [Server.PushPromise] -> IO ()) -> IO ()
testServer req _ respWriter = do
  case Server.requestPath req of
    Just "/inifite" -> do
      let infiniteBSWriter :: (Builder.Builder -> IO ()) -> IO () -> IO ()
          infiniteBSWriter bsWriter flush = do
            bsWriter $ Builder.lazyByteString $ LBS.concat $ replicate 1000 "foo\n"
            flush
            infiniteBSWriter bsWriter flush
          infiniteResponse = Server.responseStreaming status200 [] infiniteBSWriter
      respWriter infiniteResponse []
    _ -> do
      respWriter (Server.responseNoBody status404 []) []

testClient :: Client.Client ()
testClient client = client (Client.requestNoBody "GET" "/inifite" []) $ \res ->
  if Client.responseStatus res == Just status200
    then do
      bs <- Client.getResponseBodyChunk res
      putStrLn $ "Got chunk of size: " <> show (BS.length bs)
      threadDelay maxBound
    else error $ "the response isn't 200, due to a typo?"

main :: IO ()
main = do
  bracket (bindRandomPortTCP "*") (close . snd) $ \(serverPort, listenSock) -> do
    listen listenSock 1024
    _ <- async $ forever $ do
      (sock, _) <- accept listenSock
      let cleanup cfg = do
            Client.freeSimpleConfig cfg
            close sock
      async $ bracket (Server.allocSimpleConfig sock 4096) cleanup $ \cfg -> do
        Server.run cfg testServer

    let clientConfig =
          Client.ClientConfig
            { Client.scheme = "http",
              Client.authority = "localhost",
              Client.cacheLimit = 20
            }
    bracket (fst <$> getSocketTCP "localhost" serverPort) close $ \sock ->
      bracket (Client.allocSimpleConfig sock 4096) Client.freeSimpleConfig $ \http2Cfg -> do
        Client.run clientConfig http2Cfg $ testClient
    pure ()

I can see the traffic in wireshark, so this should mean that the server isn't stuck and filling up the memory. So, this has to be on the client side.

@kazu-yamamoto kazu-yamamoto self-assigned this Mar 31, 2023
@kazu-yamamoto
Copy link
Owner

This is probably because the flow control is disabled in v4.1.0
Do you see this bug also in v4.0.0?

@akshaymankar
Copy link
Author

I can confirm this doesn't happen with v4.0.0.

@akshaymankar
Copy link
Author

However, with v4.0.0, the stuck stream causes other streams to also get stuck:

Here is an updated example:

{-# LANGUAGE OverloadedStrings #-}

module Main where

import Control.Concurrent
import Control.Concurrent.Async
import Control.Exception
import Control.Monad
import qualified Data.ByteString as BS
import qualified Data.ByteString.Builder as Builder
import qualified Data.ByteString.Lazy as LBS
import Data.Streaming.Network
import Network.HTTP.Types
import qualified Network.HTTP2.Client as Client
import qualified Network.HTTP2.Server as Server
import Network.Socket

testServer :: Server.Request -> Server.Aux -> (Server.Response -> [Server.PushPromise] -> IO ()) -> IO ()
testServer req _ respWriter = do
  case Server.requestPath req of
    Just "/inifite" -> do
      let infiniteBSWriter :: (Builder.Builder -> IO ()) -> IO () -> IO ()
          infiniteBSWriter bsWriter flush = do
            bsWriter $ Builder.lazyByteString $ LBS.concat $ replicate 1000 "foo\n"
            flush
            infiniteBSWriter bsWriter flush
          infiniteResponse = Server.responseStreaming status200 [] infiniteBSWriter
      respWriter infiniteResponse []
    Just "/echo" -> do
      reqBody <- readRequestBody req
      respWriter (Server.responseBuilder status200 [] (Builder.lazyByteString reqBody)) []
    _ -> do
      respWriter (Server.responseNoBody status404 []) []

stuckClient :: Client.Client ()
stuckClient client = client (Client.requestNoBody "GET" "/inifite" []) $ \res ->
  if Client.responseStatus res == Just status200
    then do
      bs <- Client.getResponseBodyChunk res
      putStrLn $ "Got chunk of size: " <> show (BS.length bs)
      threadDelay maxBound
    else error $ "the response isn't 200, due to a typo?"

echoClient :: Client.Client ()
echoClient client = client (Client.requestBuilder "GET" "/echo" [] "Yo") $ \res ->
  if Client.responseStatus res == Just status200
    then do
      resp <- readResponseBody res
      putStrLn $ "Got echo response: " <> show resp
    else error $ "the response isn't 200, due to a typo?"

main :: IO ()
main = do
  bracket (bindRandomPortTCP "*") (close . snd) $ \(serverPort, listenSock) -> do
    listen listenSock 1024
    _ <- async $ forever $ do
      (sock, _) <- accept listenSock
      let cleanup cfg = do
            Client.freeSimpleConfig cfg
            close sock
      async $ bracket (Server.allocSimpleConfig sock 4096) cleanup $ \cfg -> do
        Server.run cfg testServer

    let clientConfig =
          Client.ClientConfig
            { Client.scheme = "http",
              Client.authority = "localhost",
              Client.cacheLimit = 20
            }
    bracket (fst <$> getSocketTCP "localhost" serverPort) close $ \sock ->
      bracket (Client.allocSimpleConfig sock 4096) Client.freeSimpleConfig $ \http2Cfg -> do
        Client.run clientConfig http2Cfg $ \sendReq -> do
          stuck <- async $ stuckClient sendReq
          threadDelay 100000
          echoClient sendReq
          putStrLn $ "!!!! echo request worked even if stuck request was stuck !!!!"
          wait stuck
    pure ()

readRequestBody :: Server.Request -> IO LBS.ByteString
readRequestBody req = readChunks (Server.getRequestBodyChunk req)

readResponseBody :: Client.Response -> IO LBS.ByteString
readResponseBody res = readChunks (Client.getResponseBodyChunk res)

readChunks :: IO BS.ByteString -> IO LBS.ByteString
readChunks action = LBS.fromChunks <$> go []
  where
    go chunks = do
      action >>= \c -> case c of
        "" -> pure chunks
        _ -> go (c : chunks)

@kazu-yamamoto
Copy link
Owner

Thank you for your research and the code. I will look into it.

@kazu-yamamoto
Copy link
Owner

With your first example and http2 v4.1.0, I cannot reproduce memory-blow-up on macOS.
I'm watching the process by ps.

Would you tell me your OS?

@akshaymankar
Copy link
Author

I am using Linux, NixOS specifically.

akshaymankar added a commit to wireapp/wire-server that referenced this issue Apr 4, 2023
http2 v4.1.0 disables flow control, this makes generator of infinite body
consume too much CPU and starve other threads, making tests slow. Giving more
threads to the test binary speeds the tests up quite significantly.

Upstream Issue: kazu-yamamoto/http2#62
akshaymankar added a commit to wireapp/wire-server that referenced this issue Apr 4, 2023
http2 v4.1.0 disables flow control, this makes generator of infinite body
consume too much CPU and starve other threads, making tests slow. Giving more
threads to the test binary speeds the tests up quite significantly.

Upstream Issue: kazu-yamamoto/http2#62
akshaymankar added a commit to wireapp/wire-server that referenced this issue Apr 11, 2023
http2 v4.1.0 disables flow control, this makes generator of infinite body
consume too much CPU and starve other threads, making tests slow. Giving more
threads to the test binary speeds the tests up quite significantly.

Upstream Issue: kazu-yamamoto/http2#62
@kazu-yamamoto
Copy link
Owner

@akshaymankar I would appreciated if you was able to test this issue to v5.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants