hGetContents closes handle

[yaitskov]

hGetContents doesn't create Handle and therefore it is not obvious why it closes it. Handle is closed on a bracket.

data GitObject
  = GitObject
    { gobHash :: !(Digest SHA1State)
    , gobOrigin :: !FilePath
    }
    deriving (Show, Eq)

mkGitObject :: PhoenixM m => FilePath -> m (Maybe GitObject)
mkGitObject fp = do
  s <- asks inHandlesSem
  U.bracket_ (U.waitQSem s) (U.signalQSem s) $
    U.withBinaryFile fp U.ReadMode $ \inH -> do
      magicBs <- hGet inH 2
      if zlibP magicBs
        then do
          !headerBs <- (magicBs <>) <$> hGet inH 10
          (`U.catch` skipCorruptedFile inH) $ do
            if gitObjectP $ Z.decompress (toLazy headerBs)
              then do
                !goh <- sha1 . Z.decompress . (toLazy headerBs <>) <$> hGetContents inH
                pure . Just $ GitObject goh fp
              else pure Nothing
        else pure Nothing
  where
    skipCorruptedFile inH (e :: Z.DecompressError) =
      case e of
        Z.TruncatedInput -> do
          fsz <- liftIO $ U.hFileSize inH

[Bodigrim]

Data.ByteString.Lazy.hGetContents implements a lazy IO and as such is not suitable to be used inside bracket. For instance,

ghci> import System.IO
ghci> import Data.ByteString.Lazy qualified as BL
ghci> withBinaryFile "foo.txt" ReadMode BL.hGetContents >>= print
"*** Exception: foo.txt: hGetBufSome: illegal operation (handle is closed)

That's because the moment BL.hGetContents runs unsafeInterleaveIO it passes control to withBinaryFile which readily closes the handle. When later on the result of Bl.hGetContents is being forced, BL.hGetContents tries to read from the closed handle and fails.

The expected way to use BL.hGetContents is to pass it a handle and let it close it on its own accord. This is the documented behaviour:

bytestring/Data/ByteString/Lazy.hs

Lines 1602 to 1603 in 10099d9

	-- File handles are closed on EOF if all the file is read, or through
	-- garbage collection otherwise.

---

A variant of BL.hGetContents as proposed in #708 which does not close the handle seems to me even more dangerous than the original: it still cannot be used under bracket because lazy IO will escape again for the same reason of unsafeInterleaveIO, but also now a developer is responsible to put hClose manually exactly at the spot where ByteString has been consumed in full. While helpful in marginal cases as presented above, when a lazy ByteString is consumed strictly and immediately, in general it would present a user with a nuanced choice between two perilous functions and as such be a net negative.

All in all lazy IO struggles when precise resource management is needed. If the latter is a requirement, I'd use Data.Conduit.Zlib, Data.Streaming.Zlib, Codec.Compression.Zlib.Internal or similar. As a maintainer of zlib I'm open to add a high level helper, something like

decompressAndReduceFromHandle :: Handle -> (acc -> S.ByteString -> IO acc) -> acc -> IO (acc, S.ByteString)

(where the second component of return stands for trailer after the end of archive).

[yaitskov]

https://github.com/yaitskov/lazy-scope