openFile is not exception safe

[treeowl]

If someone opens a file without masking exceptions, then (on a bad day) the file could be opened but no finalizer created to close it. That breaks the typical user expectation that dropping a Handle will eventually close the associated file descriptor. I fixed a related issue in base a while back, though I don't remember the details. Arguably the whole idea of automatically closing files is a bad one, but consistency seems important.

[hasufell]

@Mistuke

[Mistuke]

I don't believe the programming model was to rely on finalizers to close handles. The non determinism of when this happens can cause a number of issues.

As far as I'm aware of the automatic Handle cleanup is a last ditch effort to prevent resource leaking but not something to be relied upon. At least that's what I've alway used as the model.

[treeowl]

@Mistuke So do you think we just shouldn't put finalizers on handles?

[Mistuke]

@treeowl no, we should have them for a resource management point of view. It's always good to try not to leak resources, especially if the program is about to abort.

What I'm saying is I don't think they should be relied upon for correctness of your program. In my opinion the finalizers are there to try handle situations where the program is "incorrect". In the sense that it had no deterministic resource management structure. But a programmer shouldn't rely on them to run in order for his program to work.

But that's my opinion on the subject and how I've mostly treated them.
Note that I think it's different when the resource is internal. Like it would be ok for removal of foreign wrappers etc.

[treeowl]

@Mistuke It sounded like you thought it isn't a bug that openFile isn't exception safe, but now I can't tell .... If it's interrupted by an asynchronous exception, then there's no way whatever to close the file without exiting the program.

[Mistuke]

Sorry for the confusion. That openFile isn't exception safe is a valid bug. But I think that should an exception happen in openFile there should be no handle at all (which might be what you're saying?)

That's because I don't think

That breaks the typical user expectation that dropping a Handle will eventually close the associated file descriptor.

Isn't a valid expectation to have. But as you say users expect it, so we should try to get it to work as much as possible.

[treeowl]

If you look at what I did in base, that's one approach. Not necessarily the prettiest. the point is that we want to be sure to mask exceptions until we've installed the finalizer.

[hasufell]

I'm still not sure what is your proposal exactly. I don't know what you did in base.

[treeowl]

Here's the code to look at. See the explanation above it.

https://hackage.haskell.org/package/base-4.17.0.0/docs/src/GHC.IO.Handle.FD.html#withOpenFile%27

[treeowl]

Also read the stuff at openFileWith, which that uses.

[Bodigrim]

Breadcrumbs:

• The patch for base landed in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/4681
• It was supposed to fix several issues, including https://gitlab.haskell.org/ghc/ghc/-/issues/19114

[Bodigrim]

This works:

{-# LANGUAGE QuasiQuotes #-}

module Main where

import Control.Concurrent
import Control.Monad
import qualified Data.ByteString.Lazy as BL
import System.File.OsPath
import System.IO
import System.OsPath

main :: IO ()
main = replicateM_ 100000 $ do
  let fn = "test.txt"
  thr <- forkIO (System.IO.readFile fn >>= putStr)
  threadDelay 1
  void $ killThread thr

This fails with test.txt: withFile': resource exhausted (Too many open files), even after #11:

{-# LANGUAGE QuasiQuotes #-}

module Main where

import Control.Concurrent
import Control.Monad
import qualified Data.ByteString.Lazy as BL
import System.File.OsPath
import System.IO
import System.OsPath

main :: IO ()
main = replicateM_ 100000 $ do
  let fn = [osp|test.txt|]
  thr <- forkIO (System.File.OsPath.readFile fn >>= BL.putStr)
  threadDelay 1
  void $ killThread thr

[treeowl]

Whoops... It looks like I forgot the part where openFile needs to get changed, and only did some other stuff around there. I'll have to page this back in and try to look at it this week (probably this coming weekend, due to travel).

[hasufell]

Yes I'm aware we still leak the file descriptors (although Handles don't with the patch). I'll fix that shortly.

[hasufell]

I believe it is fixed now: #11