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

Regression in warp 3.2.10: Timeout reaper is triggered if socket is kept open #613

Closed
cocreature opened this Issue Feb 22, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@cocreature
Contributor

cocreature commented Feb 22, 2017

When building the program below against warp 3.2.9, everything works as expected. However, using 3.2.10 or 3.2.11 Thread killed by Warp's timeout reaper is printed to stderr when I open http://localhost:8080 in a browser (I tried firefox and chrome). This error does not occur using curl (even when copying the curl command provided by the browser) and it also doesn’t appear if I click fast enough on Close idle sockets on chrome://net-internals/#sockets. This strongly suggests that the problem is caused by browsers keeping sockets open for potential reuse. I don’t have the time right atm to bisect and find the exact commit that triggered this bug.

{-# LANGUAGE OverloadedStrings #-}
module Main where

import Network.HTTP.Types.Status
import Network.Wai
import qualified Network.Wai.Handler.Warp as Warp

main :: IO ()
main = do
  Warp.runSettings (Warp.setTimeout 1 $ Warp.setPort 8080 Warp.defaultSettings) $ \req respond ->
    respond $ responseLBS status200 [] "Hello World"
@cocreature

This comment has been minimized.

Contributor

cocreature commented Feb 23, 2017

I just figured out that this happens only using the threaded runtime. To make it easier to reproduce I also created a full cabal project that builds with cabal and stack and demonstrates the bug https://github.com/cocreature/warp-bug

@cocreature cocreature changed the title from Regression in warp 3.2.11: Timeout reaper is triggered if socket is kept open to Regression in warp 3.2.10: Timeout reaper is triggered if socket is kept open Feb 23, 2017

@cocreature

This comment has been minimized.

Contributor

cocreature commented Feb 23, 2017

A bisect points to 3eb5866

@snoyberg snoyberg self-assigned this Feb 23, 2017

@snoyberg

This comment has been minimized.

Member

snoyberg commented Feb 23, 2017

Good catch, and thanks for the clear repro steps. I think this is fixed with 35ef1ca, can you give it a shot?

@cocreature

This comment has been minimized.

Contributor

cocreature commented Feb 23, 2017

I can confirm that this fixes the bug for me. However, I noticed that merging #611 also made the bug disappear even without your commit. But maybe it’s just harder to trigger now, I haven’t looked at the actual changes.

It would be great if you could do a bugfix release.

@snoyberg

This comment has been minimized.

Member

snoyberg commented Feb 23, 2017

Yeah, I was curious if #611 would affect this... I've just merged master back into this branch, can you try that as well? Since these are the only changes since the last release, I feel comfortable making the release myself without waiting for @kazu-yamamoto (though Kazu's review is always appreciated).

@cocreature

This comment has been minimized.

Contributor

cocreature commented Feb 23, 2017

I've just merged master back into this branch, can you try that as well?

It still works :)

@snoyberg

This comment has been minimized.

Member

snoyberg commented Feb 23, 2017

Cool, I'll merge/release that PR once Travis goes green.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Feb 24, 2017

I think that 35ef1ca is a direct solution for 3eb5866.
So, it's OK to merge 35ef1ca.

Now I'm trying to understand the relationship with #611.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Feb 24, 2017

#611 looks good. It seems to me that including both patches are OK.

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Feb 24, 2017

Now fork is complicated and hard to read. I just pushed the refactoring code: c01f8e3

@snoyberg Please review it.

@cocreature Please test the 613-better-exception-handling branch.

@cocreature

This comment has been minimized.

Contributor

cocreature commented Feb 24, 2017

@kazu-yamamoto I can confirm that it still works.

@snoyberg snoyberg closed this in 35ef1ca Feb 24, 2017

snoyberg added a commit that referenced this issue Feb 24, 2017

Merge pull request #616 from yesodweb/613-better-exception-handling
Move exception handling to top of thread (fixes #613)

@snoyberg snoyberg removed the in progress label Feb 24, 2017

@snoyberg

This comment has been minimized.

Member

snoyberg commented Feb 24, 2017

Reviewed changes (look great), merged, and released to Hackage. Thanks!

@cocreature

This comment has been minimized.

Contributor

cocreature commented Feb 24, 2017

Thanks for fixing this so fast!

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