gopher-lua icon indicating copy to clipboard operation
gopher-lua copied to clipboard

Finalizer is not called when using runtime.SetFinalizer over *lua.LState and called Closed()

Open NKUCodingCat opened this issue 2 years ago • 1 comments

  • [x] GopherLua is a Lua5.1 implementation. You should be familiar with Lua programming language. Have you read Lua 5.1 reference manual carefully?
  • [x] GopherLua is a Lua5.1 implementation. In Lua, to keep it simple, it is more important to remove functionalities rather than to add functionalities unlike other languages . If you are going to introduce some new cool functionalities into the GopherLua code base and the functionalities can be implemented by existing APIs, It should be implemented as a library.

Please answer the following before submitting your issue:

  1. What version of GopherLua are you using? : v0.0.0-20210529063254-f4c35e4016d9
  2. What version of Go are you using? : go 1.16.9
  3. What operating system and processor architecture are you using? : Debian GNU/Linux 9.6 (stretch) On Intel(R) Xeon(R) Gold 5218
  4. What did you do? : call runtime.SetFinalizer on *lua.LState, details will be shown below
  5. What did you expect to see? : Finalizer function is called as soon as runtime.GC() is called
  6. What did you see instead? : No finalize function was called, which might suggested that somewhat memory leaking?
package luavm

import (
	"fmt"
	"runtime"
	"runtime/debug"
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	lua "github.com/yuin/gopher-lua"
)

func TestLuaVMRecycle(t *testing.T) {
	debug.SetGCPercent(-1)
	for i := 0; i < 10; i++ {
		go func() {
			lvm := lua.NewState()
			assert.NotNil(t, lvm)
			runtime.SetFinalizer(lvm, func(L *lua.LState) {
				fmt.Printf("lua vm %p was closed\n", L)
			})

			lvm.Close()
		}()

	}

	for i := 0; i < 10; i++ {
		runtime.GC()
		time.Sleep(1 * time.Second)
	}
}

func TestArrayRecycle(t *testing.T) {
	debug.SetGCPercent(-1)
	for i := 0; i < 10; i++ {
		go func() {
			ia := make([]int, 10)
			ip := &ia
			runtime.SetFinalizer(ip, func(L *[]int) {
				fmt.Printf("int array %p was closed\n", L)
			})
		}()
	}

	for i := 0; i < 10; i++ {
		runtime.GC()
		time.Sleep(1 * time.Second)
	}
}

run these two test will got

Running tool: /usr/local/go/bin/go test -timeout 30s -run ^(TestLuaVMRecycle|TestArrayRecycle)$ ****/internal/**/luavm -v

=== RUN   TestLuaVMRecycle
--- PASS: TestLuaVMRecycle (10.40s)
=== RUN   TestArrayRecycle
int array 0xc00138a0a8 was closed
int array 0xc00000e318 was closed
int array 0xc0002026a8 was closed
int array 0xc000202690 was closed
int array 0xc0001a4090 was closed
int array 0xc0001a4078 was closed
int array 0xc0001a4060 was closed
int array 0xc0001a4048 was closed
int array 0xc0001a4030 was closed
int array 0xc0001a4018 was closed
--- PASS: TestArrayRecycle (10.99s)
PASS
ok  	****/internal/**/luavm	22.156s

Am I miss something need to call except Close()? It seems there is still some memory is not collected even if I explicitly called runtime.GC() and debug.SetGCPercent(-1)

NKUCodingCat avatar Mar 10 '22 11:03 NKUCodingCat

I think this is a quirk of Go's finalisers. If you attach a finaliser to something which refers to itself (i.e. creates a GC loop) then the finaliser will never occur (in fact, I think attaching the finaliser will stop the GC ever collecting that object and thus cause a leak!)

See https://medium.com/a-journey-with-go/go-finalizers-786df8e17687

If you need to have a GC on an LState, you probably need to attach it to something that the LState references, but that doesn't reference back to the LState. Eg you could set one on an LTable, and then store the only reference to that LTable as a global in the LState.

My use of finalisers in #267 might be a useful reference too.

(TL;DR I don't believe this is a bug in Gopher Lua)

tul avatar Jul 25 '22 16:07 tul

@tul Thank you for your reply about my confusion. I do find some discussion about circular reference and finalizer https://groups.google.com/g/golang-nuts/c/tzvgeBEW1WY . It seems like runtime.setfinalizer is somewhat as-is feature and lua.LState is incompatible with it...

NKUCodingCat avatar Sep 15 '22 15:09 NKUCodingCat