Opened 21 months ago

Closed 5 months ago

#13167 closed bug (fixed)

GC and weak reference finalizers and exceptions

Reported by: Yuras Owned by:
Priority: normal Milestone: 8.6.1
Component: Compiler Version: 8.0.1
Keywords: Cc: simonmar, Phyx-
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D4693
Wiki Page:

Description

When GC runs a number of finalizers in a row, and the first of them throws an exception, then other finalizers are ignored. The relevant piece of code is here.

The following program reproduces the issue:

import Data.IORef
import Control.Monad
import Control.Exception
import System.Mem

main :: IO ()
main = do
  run
  run
  run
  run
  performMajorGC
  performMajorGC

run :: IO ()
run = do
  ref <- newIORef ()
  void $ mkWeakIORef ref $ do
    putStr "."
    throwIO $ ErrorCall "failed"

I expect it to output "....", but I get only "."

The issue makes it unsafe to rely on finalizer for resource cleanup because unrelated finalizer (e.g. from some other library) may prevent your finalizer from running.

Actually I was sure the issue is known, but today I tried to find a reference to it, and failed.

If it is by design, then it should be documented somewhere.

Change History (9)

comment:1 Changed 21 months ago by simonpj

Cc: simonmar added

Adding simonmar who is king of finalisers.

comment:2 Changed 21 months ago by simonmar

Yes, this is technically a bug. We could have runFinalizerBatch catch and discard exceptions (or print them to stdout; it's really a bug if a finalizer throws an exception).

comment:3 Changed 15 months ago by andrewthad

I've open a PR that adds docs explaining this behavior. https://github.com/ghc/ghc/pull/51

comment:4 in reply to:  2 Changed 5 months ago by Phyx-

Cc: Phyx- added

Replying to simonmar:

Yes, this is technically a bug. We could have runFinalizerBatch catch and discard exceptions (or print them to stdout; it's really a bug if a finalizer throws an exception).

Any suggestions on how? the I/O manager seems to rely on using finalizers to flush buffers, however for some reason my new I/O manager's finalizers never get called. I assume because something else threw an exception.

I've modified runFinalizerBatch to handle exceptions (unless I misunderstood the code)

runFinalizerBatch :: Int -> Array# (State# RealWorld -> State# RealWorld)
                  -> IO ()
runFinalizerBatch (I# n) arr =
   let  go m  = IO $ \s ->
                  case m of
                  0# -> (# s, () #)
                  _  -> let !m' = m -# 1# in
                        case indexArray# arr m' of { (# io #) ->
                        case catch# (\p -> (# io p, () #)) (\_ s'' -> (# s'', () #)) s of          {
                         (# s', _ #) -> unIO (go m') s'
                        }}
   in
        go n

With mio the output varies between .. and ...., so not very deterministic. with winio none of it ever runs...

Any ideas what else might be making these handlers not run at all? or how I can debug this? for winio I can imagine one of the finalizers deadlocking, an ffi call maybe. But the mio output is strange.

If we're going to rely on them for managing handles and buffers they would ideally be a bit more robust...

comment:5 Changed 5 months ago by simonmar

The example program in the description doesn't wait for the finalizer threads to complete, so it might exit while there are still finalizers alive. This is why you don't see deterministic output. Probably adding a threadDelay 1000000 at the end would be enough.

A deadlock sounds like a plausible explanation for the behaviour you're seeing.

The fix to runFinalizerBatch looks good - we should probably do that anyway (I'm sure you could construct a test case to demonstrate the problem that it fixes).

comment:6 Changed 5 months ago by Phyx-

ah indeed, threadDelay worked!

Ok I'll whip up a testcase and patch.

I'll see if I can't add some debug code to figure out which finalizer is deadlocking.

Thanks!

comment:7 Changed 5 months ago by Phyx-

Differential Rev(s): Phab:D4693
Status: newpatch

comment:8 Changed 5 months ago by Ben Gamari <ben@…>

In 01bb17fd/ghc:

Make finalizers more reliable.

Ignore any errors thrown by finalizers when running them.

This prevents a faulty finalizer from stopping the rest being called.

Test Plan: ./validate, new test T13167

Reviewers: hvr, bgamari, simonmar

Reviewed By: bgamari, simonmar

Subscribers: rwbarton, thomie, carter

GHC Trac Issues: #13167

Differential Revision: https://phabricator.haskell.org/D4693

comment:9 Changed 5 months ago by Phyx-

Milestone: 8.6.1
Resolution: fixed
Status: patchclosed
Note: See TracTickets for help on using tickets.