Skip to content

Commit

Permalink
Fixed memory leak caused by SetFinalizer
Browse files Browse the repository at this point in the history
  • Loading branch information
Seb-C committed Dec 19, 2023
1 parent 0683003 commit ff5481f
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 58 deletions.
39 changes: 39 additions & 0 deletions deferred.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package ex

import (
"fmt"
)

type deferred struct {
callback func() error
fromFile string
fromLine int
gcUnclosedDetector *gcUnclosedDetector
}

func newDeferred(
callback func() error,
fromFile string,
fromLine int,
) *deferred {
deferred := &deferred{
callback: callback,
fromFile: fromFile,
fromLine: fromLine,
}
deferred.gcUnclosedDetector = newGCUnclosedDetector(deferred.String())

return deferred
}

func (deferred *deferred) String() string {
if deferred.fromFile == "" {
return "deferred close"
}

return fmt.Sprintf(
"deferred close initiated by %s:%d",
deferred.fromFile,
deferred.fromLine,
)
}
52 changes: 52 additions & 0 deletions gc_unclosed.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package ex

import (
"fmt"
"os"
"runtime"
)

var onGarbageCollectUnclosed = func(err error) {
fmt.Fprintln(os.Stderr, err)
}

// OnGarbageCollectUnclosed changes what happens when a Terminator
// gets garbage collected without the Close method having been called.
//
// The default behaviour is to write an error message to stderr.
// Use this for example if you prefer to panic or pass it to your log
// system instead of stderr.
func OnGarbageCollectUnclosed(handler func(error)) {
if handler == nil {
panic("Cannot set a nil garbage collect unclosed handler")
}

onGarbageCollectUnclosed = handler
}

type gcUnclosedDetector struct{
description string
isClosed bool
}

func newGCUnclosedDetector(description string) *gcUnclosedDetector {
detector := &gcUnclosedDetector{
description: description,
isClosed: false,
}

runtime.SetFinalizer(detector, (*gcUnclosedDetector).finalizer)

return detector
}

func (detector *gcUnclosedDetector) finalizer() {
if detector.isClosed {
return
}

onGarbageCollectUnclosed(fmt.Errorf(
"The Close method of the terminator containing a %s was never called before being garbage collected.",
detector.description,
))
}
61 changes: 3 additions & 58 deletions terminator.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,58 +3,9 @@ package ex
import (
"errors"
"fmt"
"os"
"runtime"
)

var onGarbageCollectUnclosed = func(err error) {
fmt.Fprintln(os.Stderr, err)
}

// OnGarbageCollectUnclosed changes what happens when a Terminator
// gets garbage collected without the Close method having been called.
//
// The default behaviour is to write an error message to stderr.
// Use this for example if you prefer to panic or pass it to your log
// system instead of stderr.
func OnGarbageCollectUnclosed(handler func(error)) {
if handler == nil {
panic("Cannot set a nil garbage collect unclosed handler")
}

onGarbageCollectUnclosed = handler
}

type deferred struct {
callback func() error
fromFile string
fromLine int
isClosed bool
}

func (deferred *deferred) String() string {
if deferred.fromFile == "" {
return "deferred close"
}

return fmt.Sprintf(
"deferred close initiated by %s:%d",
deferred.fromFile,
deferred.fromLine,
)
}

func (deferred *deferred) finalizer() {
if deferred.isClosed {
return
}

onGarbageCollectUnclosed(fmt.Errorf(
"The Close method of the terminator containing a %s was never called before being garbage collected.",
deferred.String(),
))
}

// Terminator should be embedded into any struct that is responsible
// for resources that has to be closed.
type Terminator struct {
Expand All @@ -74,15 +25,8 @@ func (terminator *Terminator) Defer(callback func() error) {
// runtime information cannot be retrieved, so it can stay empty.
_, file, line, _ := runtime.Caller(1)

deferredClose := &deferred{
callback: callback,
fromFile: file,
fromLine: line,
isClosed: false,
}
deferredClose := newDeferred(callback, file, line)
terminator.defers = append(terminator.defers, deferredClose)

runtime.SetFinalizer(deferredClose, (*deferred).finalizer)
}

// Close will execute all the deferred functions that were previously passed to Defer.
Expand All @@ -102,8 +46,9 @@ func (terminator *Terminator) Close() error {
if err := deferred.callback(); err != nil {
errs = append(errs, fmt.Errorf("%s, caused by %w", deferred.String(), err))
}
deferred.isClosed = true
deferred.gcUnclosedDetector.isClosed = true
}
terminator.defers = nil

return errors.Join(errs...)
}
27 changes: 27 additions & 0 deletions terminator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package ex_test

import (
"errors"
"fmt"
"io"
"regexp"
"runtime"
"testing"
Expand Down Expand Up @@ -208,5 +210,30 @@ func TestTerminator(t *testing.T) {
runtime.GC()
assert.Equal(t, 1, unclosedErrorCount)
})
t.Run("does not prevent GC", func(t *testing.T) {
type typeA struct {
ex.Terminator
}

objA1 := &typeA{}
{
objA1Ref := objA1
objA1.Defer(func() error {
// Force keeping a reference to itself inside a defer function
fmt.Fprint(io.Discard, objA1Ref)
return nil
})
}

garbageCollectedCount := 0
ex.OnGarbageCollectUnclosed(func(err error) {
garbageCollectedCount++
t.Log(err)
})

objA1 = nil // Removing the reference so that it gets garbage collected
runtime.GC()
assert.Equal(t, 1, garbageCollectedCount)
})
})
}

0 comments on commit ff5481f

Please sign in to comment.