snoyberg/http-client — pull request #13 by bitonic (2013-12-10 16:27:42+00:00)
Title: Catch the right amount of exceptions when closing connections.

This took me a while to track down, and I'm quite sure the bug has been there for a while.

We were sometimes getting "resource vanished" errors in an [http-conduit powered application](https://github.com/erudify/sssp). I managed to reliably reproduce them with this patch, which waits one second before getting the response:

```
diff --git a/http-client/Network/HTTP/Client/Core.hs b/http-client/Network/HTTP/Client/Core.hs
index b79fe8e..9d9b0da 100644
--- a/http-client/Network/HTTP/Client/Core.hs
+++ b/http-client/Network/HTTP/Client/Core.hs
@@ -26,6 +26,8 @@ import qualified Data.ByteString.Lazy as L
import Data.Monoid
import Control.Monad (void)

+import Control.Concurrent (threadDelay)
+
-- | Perform a @Request@ using a connection acquired from the given @Manager@,
-- and then provide the @Response@ to the given function. This function is
-- fully exception safe, guaranteeing that the response will be closed when the
@@ -82,6 +84,7 @@ httpRaw req' m = do
ex <- try $ do
requestBuilder req ci

+ threadDelay 1000000
getResponse connRelease timeout' req ci

case (ex, isManaged) of
````

The problem, as the comment in the commit says, is that the socket might be already closed when we try to shutdown SSL, which involves writing/reading on said socket.

If you request repeatedly using the patched `http-client` an `https` server which at some point closes the connection, you'll get a resource vanished. Sadly I can't share my test http endpoint (a private amazon S3 bucket), but this was my test case:

````
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE ScopedTypeVariables #-}
import Control.Concurrent (threadDelay)
import Control.Monad (forM_)
import Network.HTTP.Client
import Network.HTTP.Client.TLS
import Network.HTTP.Types

request :: Manager -> String -> IO ()
request manager url = do
putStrLn $ "requesting..."
req <- parseUrl url
withResponse req{method = methodHead} manager $ const (return ())

main :: IO ()
main = do
manager <- newManager tlsManagerSettings
forM_ ([1..10] :: [Int]) $ \_ -> do
request manager "https://"
threadDelay 2000000
````

Note that the `threadDelay` above is needed to trigger the remote connection closure.

The second commit is simply so that only `IOException`s are catched when using `safeCloseConn`---catching `SomeException` seems like a bad idea. An alternative to patching the `http-client-tls` is to use `safeCloseConn` directly in the Manager:

````
diff --git a/http-client/Network/HTTP/Client/Manager.hs b/http-client/Network/HTTP/Client/Manager.hs
index be70088..02bfb65 100644
--- a/http-client/Network/HTTP/Client/Manager.hs
+++ b/http-client/Network/HTTP/Client/Manager.hs
@@ -292,7 +292,7 @@ getManagedConn man key open = mask $ \restore -> do
toReuse <- I.readIORef toReuseRef
restore $ case toReuse of
Reuse -> putSocket man key ci
- DontReuse -> connectionClose ci
+ DontReuse -> safeConnClose ci

return (connRelease, ci, isManaged)

````

In general the Manager machinery seems quite fragile and prone to timing related breakage...