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

streams in constant memory seems invalid #931

Open
phadej opened this issue Mar 24, 2018 · 0 comments

Comments

Projects
None yet
1 participant
@phadej
Copy link
Member

commented Mar 24, 2018

replicate_ n (res void) is wrong thing to do. res should be called only once, and processing down inside the continuaton. See e.g. how runResultStream is made.

I suspect that the problem is with test itself, hopefully. I tried run it threaded RTS, still same result. (startWaiApp forks a new thread for test server).

Note chunks read by client aren't 5 MB in size, in fact on my machine they are most < 1k.

For some reason when running the test there are Lazy ByteString chunks visible. That's fishy.

screenshot from 2018-03-24 12-35-30

Consuming whole response isn't good, as GC will have opportunity to clean everything. Better approach would be to check maximum residency before the test, and check that it haven't grown (or bigger than something the test expected to use).

cc @jkarni

diff --git a/servant-client/test/Servant/StreamSpec.hs b/servant-client/test/Servant/StreamSpec.hs
index ad4a266..e4aa823 100644
--- a/servant-client/test/Servant/StreamSpec.hs
+++ b/servant-client/test/Servant/StreamSpec.hs
@@ -1,3 +1,4 @@
+{-# LANGUAGE BangPatterns           #-}
 {-# LANGUAGE CPP                    #-}
 {-# LANGUAGE ConstraintKinds        #-}
 {-# LANGUAGE DataKinds              #-}
@@ -83,7 +84,7 @@ server = serve sapi
     lotsGenerator f r = do
       f ""
       withFile "/dev/urandom" ReadMode $
-        \handle -> streamFiveMBNTimes handle 1000 r
+        \handle -> streamFiveMBNTimes handle 10 r
       return ()
 
     streamFiveMBNTimes handle left sink
@@ -93,8 +94,6 @@ server = serve sapi
           sink msg
           streamFiveMBNTimes handle (left - 1) sink
 
-
-
 {-# NOINLINE manager' #-}
 manager' :: C.Manager
 manager' = unsafePerformIO $ C.newManager C.defaultManagerSettings
@@ -126,11 +125,21 @@ streamSpec = beforeAll (CS.startWaiApp server) $ afterAll CS.endWaiApp $ do
        runResultStream res `shouldReturn` (jra, jrb, jra, Nothing)
 
     it "streams in constant memory" $ \(_, baseUrl) -> do
-       Right (ResultStream res) <- runClient getGetALot baseUrl
-       let consumeNChunks n = replicateM_ n (res void)
-       consumeNChunks 900
+       Right res <- runClient getGetALot baseUrl
+       n <- consumeAll res
+       print n
        memUsed <- currentBytesUsed <$> getGCStats
        memUsed `shouldSatisfy` (< (megabytes 20))
 
+-- | Consume whole 'ResultStream' calculating it's length.
+consumeAll :: ResultStream BS.ByteString -> IO Int
+consumeAll (ResultStream k) = k (go 0) where
+    go !n act = do
+        x <- act
+        case x of
+            Just (Right bs) -> go (n + BS.length bs) act
+            Just (Left err) -> fail err
+            Nothing         -> return n
+
 megabytes :: Num a => a -> a
-megabytes n = n * (1000 ^ 2)
+megabytes n = n * (1000 ^ (2 :: Int))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.