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

Fix spaceleak in sendResponse #586

Merged
merged 1 commit into from Nov 8, 2016

Conversation

Projects
None yet
3 participants
@edsko
Contributor

edsko commented Oct 28, 2016

Consider the following simple WAI application that uses wai-conduit to generate a streaming body:

{-# OPTIONS_GHC -fno-full-laziness #-}
module Main (main) where

import Data.ByteString.Builder (Builder, int32BE, word8)
import Data.Conduit
import Data.Int (Int32)
import Network.HTTP.Types
import Network.Wai.Handler.Warp (run)
import Network.Wai.Conduit

main :: IO ()
main = run 3000 $ \_req k -> k $ responseSource ok200 [] (testSource 1000000)

testSource :: Int32 -> Source IO (Flush Builder)
testSource 0  = return ()
testSource !i = do
    yield (Chunk $ int32BE i)
    yield (Chunk $ word8 10)
    yield Flush
    testSource (i - 1)

We'd expect this to run in constant space, but sadly it suffers from a space leak. In fact, it's another dreaded of-type-Pipe space leak :-( It seems I'm spending a significant chunk of my time tracking those down lately... I wish we had a better way to deal with these.

Anyway, the problem is not with the conduit per se; connecting it directly to a sink that writes all values to standard output runs in constant space. Nor is the problem in wai-conduit; manually executing the StreamingBody generated by responseSource works just fine. In the end I traced this down to a bug in sendResponse in warp itself; hopefully the commit is clear enough: we're hanging on to response longer than we need, which is causing the space leak.

Though I realize that that is somewhat hand-wavy, once I realized that the problem was here I didn't investigate in more detail the exact nature of this spaceleak. Note however that for this test case, disabling full laziness is necessary even with the bugfix applied (without the bugfix there is a spaceleak with full laziness enabled or not).

@snoyberg

This comment has been minimized.

Member

snoyberg commented Oct 29, 2016

Ouch, sorry you keep hitting these 😢

@kazu-yamamoto I see no downside to this change, how about you?

@edsko

This comment has been minimized.

Contributor

edsko commented Oct 30, 2016

Haha :) Not your fault, obviously. Just makes me wonder if there isn't something more fundamental that we can do about these problems. I'd much prefer neither library authors nor library users would need to think quite so hard about this kind of memory behaviour of libraries like conduit. Oh well. Wishful thinking right now. Maybe on day :)

@edsko

This comment has been minimized.

Contributor

edsko commented Oct 31, 2016

By the way, I don't think there can be a downside, because this just forces the evaluation of response :: Response to whnf, but the next line calls sendRsp which is strict in its Rsp argument, and that Rsp argument is constructed by looking at the top-level of the Response argument; so that evaluation of response to whnf will happen immediately anyway.

@edsko

This comment has been minimized.

Contributor

edsko commented Oct 31, 2016

@kosmikus points out that my comment about full laziness is somewhat unclear, so let me clarify: with the bug fix enabled, the test case still needs to be compiled with full laziness disabled (otherwise that testSource 1000000 expression gets floated to the top, becomes a CAF, and we get an -- uninteresting -- space leak for that reason). Full laziness does not need to be disabled in warp itself, as far as I know.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Oct 31, 2016

I'm not a big fun of lazy evaluation. So, I welcome strictness.
What about using BangPatterns instead of evaluate?

     !ret = ...
@edsko

This comment has been minimized.

Contributor

edsko commented Nov 8, 2016

I guess that would work as well; I used evaluate because it makes it a lot clearer when exactly the evaluation happens. If you have a big function definition with a lot of where-bound variables and a bang on one of them, it's harder to see. (Also, I think having the bang does in fact change the semantics of the function, as it now forces the evaluation to happen in both cases of the conditional. Don't think that actually matters but it might.) Another alternative might be to move that definition of ret up and then use

let !ret = ...
...

that would also work, and still be clear. Whatever the preferred style is :)

@kazu-yamamoto kazu-yamamoto merged commit 7f530d6 into yesodweb:master Nov 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Nov 8, 2016

I have merged your pull request and changed the style to use BangPatterns at where.
I like this style because of the case where other valuables need BangPatterns.

@edsko

This comment has been minimized.

Contributor

edsko commented Nov 8, 2016

Ok, sure. Thanks!

@edsko edsko deleted the well-typed:pr/fix-spaceleak-in-sendResponse branch Nov 8, 2016

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Nov 8, 2016

A new version has been released. Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment