Opened 8 years ago

Closed 7 years ago

Last modified 6 years ago

#1364 closed merge (fixed)

Finalizers not guaranteed to run before the program exits

Reported by: tomac@… Owned by: igloo
Priority: normal Milestone: 6.10.2
Component: Runtime System Version: 6.6.1
Keywords: Cc: pho@…, conal@…, johan.tibell@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

In the following code (compiled with compiled with ghc --make -fffi -fvia-c Test.hs) the finalizer never gets called, even after replacing printf with something else:

Test.hs:

module Main where

import Foreign.Ptr
import Foreign.ForeignPtr
import Foreign.Marshal.Utils

foreign import ccall safe "ctest.h &ctest" ctestPtr :: FunPtr (Ptr Int -> IO ())

test :: Int -> IO ()
test i = with i test'
    where
        test' ptr = do fptr <- newForeignPtr ctestPtr ptr
                       putStrLn "test"

main = do putStrLn "before test..."
          test 33
          putStrLn "after test..."

ctest.h:

#include <stdio.h>

static inline void ctest( int *i )
{
    printf( "finalizer called with: %d\n", *i );
}

I've asked about this on IRC and the Haskell Cafe mailing list and received confirmation that in GHC there is no guarantee that finalizers will run before the program exits:

http://www.haskell.org/pipermail/haskell-cafe/2007-May/025458.html

The FFI addendum to the Haskell 98 report states in section 5.5 that "The only guarantee [for finalizers] is that the finalizer runs before the program terminates".

The same statement is made in GHC documentation for newForeignPtr.

This could be a bug or merely a documentation issue but something should probably be done about it. I suspect it's a bug considering the guarantee specified in the FFI addendum isn't met.

Attachments (4)

finalizertest.tar.gz (693 bytes) - added by Svarog 7 years ago.
fix1364-ghc.patch (53.0 KB) - added by Svarog 7 years ago.
fix1364-base.2.patch (28.6 KB) - added by Svarog 7 years ago.
fix1364-testsuite.patch (8.8 KB) - added by Svarog 7 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 follow-up: Changed 8 years ago by simonmar

  • Component changed from libraries/base to Runtime System
  • difficulty changed from Unknown to Moderate (1 day)
  • Milestone set to 6.1
  • Type changed from bug to feature request

GHC has support for registering Haskell functions as finalizers, see the module Foreign.Concurrent. The problem is that it isn't practical to guarantee that finalizers all run before program exit when the finalizers themselves are Haskell code (The Boehm paper on finalizers has the rationale: http://citeseer.ist.psu.edu/boehm03destructors.html).

However, we could make an exception for C finalizers registered via Foreign.ForeignPtr, and give these extra guarantees. This has been requested before, and I think it would be a useful feature.

comment:2 Changed 7 years ago by PHO

  • Cc pho@… added

comment:3 in reply to: ↑ 1 Changed 7 years ago by conal

  • Cc conal@… added

Replying to simonmar:

However, we could make an exception for C finalizers registered via Foreign.ForeignPtr, and give these extra guarantees. This has been requested before, and I think it would be a useful feature.

As far as I can tell, running finalizers promptly is essential to efficient functional graphics programming on modern hardware. For graphics, C-only finalizers (for graphics resource deallocation) would be a perfectly fine restriction to live with.

Could we get C finalizers executed promptly? For instance, could they be executed at the end of each GC cycle, rather than thread-enqueued?

comment:4 Changed 7 years ago by simonmar

  • Milestone changed from 6.10 branch to 6.10.1

comment:5 Changed 7 years ago by Svarog

  • Owner set to Svarog

comment:6 follow-ups: Changed 7 years ago by Svarog

  • Owner changed from Svarog to simonmar

Added patches that add C finalizers that can be run directly from the garbage collector.

Changed 7 years ago by Svarog

comment:7 in reply to: ↑ 6 Changed 7 years ago by Svarog

Replying to Svarog:

Added patches that add C finalizers that can be run directly from the garbage collector.

Just in case it's not clear, fix1364-ghc.patch should be applied to ghc and fix1364-base.patch should be applied to the base library.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by tibbe

  • Cc johan.tibell@… added

Replying to Svarog:

Added patches that add C finalizers that can be run directly from the garbage collector.

Will this add a performance penalty (i.e. extra GC overhead) to code that does not make use of finalizers?

comment:9 in reply to: ↑ 8 Changed 7 years ago by Svarog

Replying to tibbe:

Will this add a performance penalty (i.e. extra GC overhead) to code that does not make use of finalizers?

I'm pretty sure that it won't. The only overhead is that the weak pointer struct now has an extra field for a C finalizer and the code that goes through dead weak pointers and schedules existing Haskell finalizers has an extra if statement to check for C finalizers and run them immediately.

comment:10 follow-up: Changed 7 years ago by simonmar

I'm validating the code now. Any chance you could whip up a test for the testsuite?

comment:11 in reply to: ↑ 10 Changed 7 years ago by Svarog

Replying to simonmar:

I'm validating the code now. Any chance you could whip up a test for the testsuite?

I've just attached a patch for the testsuite and an updated patch for the finalizers.

comment:12 follow-up: Changed 7 years ago by simonmar

Sorry for this late review, and I'm not sure why I didn't pick this up before, but I think this patch has some problems.

  • we shouldn't be extending the Foreign.ForeignPtr API, as it is set in stone by the FFI spec and would need at the very least a library proposal to change it. I had imagined that we would keep the same API, and just implement C finalizers differently.
  • The implementation doesn't seem to respect the ordering of finalizers. Finalizers are supposed to run in the same order that they were added to the ForeignPtr (which is why we have that horrible IORef attached to ForeignPtrs). It looks like C finalizers will run in an arbitrary order.

Once again, I'm sorry for picking this up so late. I thought I'd reviewed the patch earlier, but I guess I didn't.

comment:13 in reply to: ↑ 12 Changed 7 years ago by Svarog

Replying to simonmar:

I've just uploaded updated patches for the base lib, the testsuite and ghc to address these issues.

The new ghc patch just adds a comment in Weak.c to mention that finalizers rely on weak pointers in weak_ptr_list always being in the same order (which is currently the case).

  • we shouldn't be extending the Foreign.ForeignPtr API, as it is set in stone by the FFI spec and would need at the very least a library proposal to change it. I had imagined that we would keep the same API, and just implement C finalizers differently.

As discussed on IRC, addForeignPtrFinalizer only needs to work with C functions so I renamed addForeignPtrFinalizerC to addForeignPtrFinalizer. That way the API stays the same. To use Haskell finalizers addForeignPtrConcFinalizer should be used.

  • The implementation doesn't seem to respect the ordering of finalizers. Finalizers are supposed to run in the same order that they were added to the ForeignPtr (which is why we have that horrible IORef attached to ForeignPtrs). It looks like C finalizers will run in an arbitrary order.

Weak pointers are always kept in the same order and each C finalizer is attached to its own weak pointer. All C finalizers attached to a particular ForeignPtr should execute in correct order. I've updated one of the tests in the testsuite to check for this.

As discussed on IRC I added code that throws a run-time error if Haskell and C finalizers are mixed on the same ForeignPtr. I've added another test to the testsuite to check this as well.

comment:14 Changed 7 years ago by simonmar

Perhaps I'm reading the code wrong, but it seems that you'll get two weak pointers created by a newForeignPtr: one created by addForeignPtrFinalizer and one created by addForeignPtrConcFinalizer (which is called by addForeignPtrFinalizer). Also, the weak pointer with the C finalizer should be attached to the MutVar# inside the IORef, in the same way that addForeignPtrConcFinalizer does (this is a safety measure, because the compiler might optimise away ForeignPtrContents, but it can't optimise away a MutVar#).

comment:15 follow-up: Changed 7 years ago by simonmar

  • Architecture changed from Unknown to Unknown/Multiple

comment:16 Changed 7 years ago by simonmar

  • Operating System changed from Unknown to Unknown/Multiple

Changed 7 years ago by Svarog

Changed 7 years ago by Svarog

Changed 7 years ago by Svarog

comment:17 in reply to: ↑ 15 Changed 7 years ago by Svarog

Replying to simonmar:

Yes, you are right. I've rewritten the patches from scratch and hopefully the new patches address all the problems.

comment:18 Changed 7 years ago by igloo

  • Milestone changed from 6.10.1 to 6.10.2

comment:19 Changed 7 years ago by simonmar

  • Owner changed from simonmar to igloo
  • Type changed from feature request to merge

Ok, I've finally reviewed, amended, tested and pushed these patches. Under normal circumstances we wouldn't push this into the stable branch, because it constitutes a new feature and makes a minor API change. However, there is strong demand for this feature, and the API change is really very minor (a new primop).

Patches to merge: in GHC:

Wed Dec 10 15:04:25 GMT 2008  Simon Marlow <[email protected]>
  * FIX #1364: added support for C finalizers that run as soon as the value is not longer reachable.
    
  Patch originally by Ivan Tomac <[email protected]>, amended by
  Simon Marlow:
  
    - mkWeakFinalizer# commoned up with mkWeakFinalizerEnv#
    - GC parameters to ALLOC_PRIM fixed

to libraries/base:

Wed Dec 10 15:05:10 GMT 2008  Ivan Tomac <[email protected]>
  * FIX #1364: added support for C finalizers that run as soon as the value is no longer reachable.
  
  Patch amended by Simon Marlow:
    - mkWeakFinalizer# commoned up with mkWeakFinalizerEnv#

comment:20 Changed 7 years ago by igloo

  • Resolution set to fixed
  • Status changed from new to closed

Both merged.

comment:21 Changed 6 years ago by simonmar

  • difficulty changed from Moderate (1 day) to Moderate (less than a day)
Note: See TracTickets for help on using tickets.