for and repeat different behaviour producing data races

[thomas-tacquet]

• [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? : github.com/yuin/gopher-lua v0.0.0-20200816102855-ee81675732da (in my go.mod)
2. What version of Go are you using? : 1.16.3
3. What operating system and processor architecture are you using? : linux amd64
4. What did you do? : I noticed a different behavior while calling a custom function in a for loop and in a repeat loop. Run the following program with this command : go run . -race and change lua variables for testing :

testCallsInRow = true
testFor = true
testRepeat = true

Row case and for case are fine, but repeat always generate a race condition.

Here is my test :

package main

import (
	"fmt"
	"sync"

	lua "github.com/yuin/gopher-lua"
)

// my map of functions
var luaFuncs = map[string]lua.LGFunction{
	"register_callback": regPlayEvt,
	"play":              play,
}

// L is global lua state
var L *lua.LState

// callbackMtx is used to protect multiple calls of my function
var callbackMtx sync.Mutex

func main() {
	// creating new state
	L = lua.NewState()

	// registering my module to have play & register_callback accessible in lua
	mod := L.RegisterModule("my", luaFuncs)
	L.Push(mod)

	defer L.Close()
	if err := L.DoString(`
	-- myCallback is juste a simple but not empty function that will be called
	function myCallback()
		print("completed")
	end;

	-- here I register myCallback
	my.register_callback(myCallback)

	-- variables for enabling/disabling testing
	testCallsInRow = true
	testFor = true
	testRepeat = true
	
	-- this test just call my play function in 'raw' style
	if testCallsInRow == true then
		my.play()
		my.play()
		my.play()
		my.play()
		my.play()
	end
	
	-- this test calls my function in a loop, same behaivior as raw style
	if testFor == true then
		for i=6,0,-1
		do
			my.play()
		end
	end

	-- this test calls my function in a repeat loop, it causes dataraces
	if testRepeat == true then
		i=0
		
		repeat
			my.play()
			i=i+1
		until i>4
	end

`); err != nil {
		panic(err)
	}
}

// onCallback is my callback that will be called in a goroutine on each play
var onCallback *lua.LFunction

// regPlayEvt enregistre la callback à appeler au démarrage de lecture
func regPlayEvt(l *lua.LState) int {
	callbackFun := l.CheckFunction(1)
	l.Pop(1)

	if callbackFun != nil {
		onCallback = callbackFun
	}

	return 1
}

// play just print a text an then create a goroutine where my lua callback function will be called
func play(_ *lua.LState) int {
	fmt.Println("playing...")

	go func(l *lua.LState) {
		// protecting from multiple calls
		callbackMtx.Lock()
		defer callbackMtx.Unlock()

		// creating a new thread to avoid concurrent access to local lua state (named L)
		localL, _ := l.NewThread()
		if err := localL.CallByParam(lua.P{
			Fn:      onCallback,
			NRet:    0,
			Protect: true,
		}, nil); err != nil {
			l.RaiseError(err.Error())
		}
	}(L)

	return 1
}

5. What did you expect to see? :

Same behavior in row, for and repeat : no race conditions as my callbacks seems safe, and doesn't cause errors on for loops.

6. What did you see instead? :

I always see a race condition in repeat case. But it looks like that repeat and for have a different implementation and I can't tell why only the repeat statement case (testRepeat = true) throws a race condition when race detector is enabled.

Is my implementation bad or there are differences in for vs repeat implementations ?

[waschik]

If I run go run -race . (note I had to move . at the end) I get:

Write at 0x00c00011c480 by main goroutine:
[...]
[...]/00318.go:31 +0x196
[...]
Previous read at 0x00c00011c480 by goroutine 13:
[...]/00318.go:103 +0x196

So the main DoString has a race condition with the play goroutine. You make only sure that play does not run parallel. But the main goroutine continues while one play goroutine runs. The documentation writes:

The LState is not goroutine-safe. It is recommended to use one LState per goroutine and communicate between goroutines by using channels.

Okay you are creating a new LState with NewThread which is used for coroutines. But comment for this function also writes:

NewThread returns a new LState that shares with the original state all global objects.

You should be only safe to use LStates which do not share writable data in different goroutines (created with NewState). In your case it is maybe the global i in the repeat loop which cause trouble but if something is not goroutine safe it could be anything.