search

goby-lang / goby

Goby - Yet another programming language written in Go
MIT License
3.49k stars 171 forks source link

Concurrent access to variable causes a race condition detection #499

Open 64kramsystem opened 6 years ago

64kramsystem commented 6 years ago

Observed in #493:

When concurrently access a variable, with the race detector, a race condition is detected.

I don't think that race conditions in the interpreted language should reflect in race conditions in the VM, although I'm not entirely sure without a good knowledge of the VM.

Test case:

$ go build -race
$ goby -i
Goby 0.1.3 🤑 😭 👽
» finish_message = nil
#» nil
»
» thread do
¤   finish_message = "thread"
» end
#» nil
»
» finish_message
==================
WARNING: DATA RACE
Read at 0x00c4201b80e0 by main goroutine:
  github.com/goby-lang/goby/vm.(*VM).GetREPLResult()
      /path/to/go/src/github.com/goby-lang/goby/vm/repl.go:60 +0x87
  github.com/goby-lang/goby/igb.StartIgb()
      /path/to/go/src/github.com/goby-lang/goby/igb/repl.go:232 +0x7bc
  main.main()
      /path/to/go/src/github.com/goby-lang/goby/goby.go:29 +0xb74

Previous write at 0x00c4201b80e0 by goroutine 12:
  github.com/goby-lang/goby/vm.(*baseFrame).insertLCL()
      /path/to/go/src/github.com/goby-lang/goby/vm/call_frame.go:120 +0x85
  github.com/goby-lang/goby/vm.glob..func9()
      /path/to/go/src/github.com/goby-lang/goby/vm/instruction.go:163 +0x253
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:119 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:56 +0x10e
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).builtinMethodYield()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:135 +0x4c4
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func18.1.1()
      /path/to/go/src/github.com/goby-lang/goby/vm/class.go:1045 +0x60

Goroutine 12 (finished) created at:
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func18.1()
      /path/to/go/src/github.com/goby-lang/goby/vm/class.go:1044 +0x3fa
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:66 +0x531
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).evalBuiltinMethod()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:232 +0x66d
  github.com/goby-lang/goby/vm.glob..func25()
      /path/to/go/src/github.com/goby-lang/goby/vm/instruction.go:476 +0x795
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:119 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:56 +0x10e
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*VM).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/vm.go:271 +0x4e
  github.com/goby-lang/goby/vm.(*VM).REPLExec()
      /path/to/go/src/github.com/goby-lang/goby/vm/repl.go:43 +0xcf2
  github.com/goby-lang/goby/igb.StartIgb()
      /path/to/go/src/github.com/goby-lang/goby/igb/repl.go:230 +0x7ab
  main.main()
      /path/to/go/src/github.com/goby-lang/goby/goby.go:29 +0xb74
==================
==================
WARNING: DATA RACE
Read at 0x00c4201b8268 by main goroutine:
  github.com/goby-lang/goby/vm.(*StringObject).toString()
      /path/to/go/src/github.com/goby-lang/goby/vm/string.go:1636 +0x3c
  github.com/goby-lang/goby/vm.(*VM).GetREPLResult()
      /path/to/go/src/github.com/goby-lang/goby/vm/repl.go:60 +0x9d
  github.com/goby-lang/goby/igb.StartIgb()
      /path/to/go/src/github.com/goby-lang/goby/igb/repl.go:232 +0x7bc
  main.main()
      /path/to/go/src/github.com/goby-lang/goby/goby.go:29 +0xb74

Previous write at 0x00c4201b8268 by goroutine 12:
  github.com/goby-lang/goby/vm.(*VM).initObjectFromGoType()
      /path/to/go/src/github.com/goby-lang/goby/vm/string.go:1616 +0x1c66
  github.com/goby-lang/goby/vm.glob..func20()
      /path/to/go/src/github.com/goby-lang/goby/vm/instruction.go:337 +0x87
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:119 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:56 +0x10e
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).builtinMethodYield()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:135 +0x4c4
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func18.1.1()
      /path/to/go/src/github.com/goby-lang/goby/vm/class.go:1045 +0x60

Goroutine 12 (finished) created at:
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func18.1()
      /path/to/go/src/github.com/goby-lang/goby/vm/class.go:1044 +0x3fa
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:66 +0x531
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).evalBuiltinMethod()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:232 +0x66d
  github.com/goby-lang/goby/vm.glob..func25()
      /path/to/go/src/github.com/goby-lang/goby/vm/instruction.go:476 +0x795
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:119 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:56 +0x10e
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*VM).startFromTopFrame()
      /path/to/go/src/github.com/goby-lang/goby/vm/vm.go:271 +0x4e
  github.com/goby-lang/goby/vm.(*VM).REPLExec()
      /path/to/go/src/github.com/goby-lang/goby/vm/repl.go:43 +0xcf2
  github.com/goby-lang/goby/igb.StartIgb()
      /path/to/go/src/github.com/goby-lang/goby/igb/repl.go:230 +0x7ab
  main.main()
      /path/to/go/src/github.com/goby-lang/goby/goby.go:29 +0xb74
st0012 commented 6 years ago

Um...I don't think it'd be a test framework issue, should be something wrong in the interpreter.

st0012 commented 6 years ago

It seems that the race condition is caused by assigning the string object "thread" to the finish_message pointer's value, while we're reading that pointer's value after evaluating a call frame for checking error objects. I think the best solution is to change the way we do error checking, I'll try to fix this in this week.

st0012 commented 6 years ago

@saveriomiroddi I think I might need your help on this. We sometimes might have race condition on pointer's target read/assignment. In current implementation this might happen when we assign values to the var finish_message at thread A, and checking if the top pointer (which is also var finish_message)'s value is an error at thread B. I'm not familiar with the pointer's thread safety, should we also need to implement something like share_ptr in c++?

BTW if we just put a fmt.Println("123") inside thread's execInstruction function then everything will work fine, I don't know why

64kramsystem commented 6 years ago

Very interesting. I'll review this over the next few days!

64kramsystem commented 6 years ago

Hello! This is a very interesting subject. As a general observation, I think this is the typical reasons why scripting interpreters use the GIL :smile:

Having said that, I had a cursory look around at the general patterns for handling shared access to pointers (which are not thread safe in Go).

I think that the high level behavior of the code would reflect the low-level one (note that I didn't read the source code yet) - in other words, at high level, there is a shared access to a variable, at low level, to a pointer.

If this is the case, there are two options:

  1. use sync.Once, which includes the boilerplate for variables shared access; this works as expected, but is in general discouraged in Go
  2. use thread channels; this is encouraged in Go.

Not knowing Go well, and maybe, considering it as easier solution, I'd use 1.

st0012 commented 6 years ago

@saveriomiroddi Actually after thinking for a few days, I think this case is acceptable. Yes, it might have a race condition on pointer's target, but that's what users should expect when writing such code. And the error is raised because the potential race condition is detected, not because there's something went wrong. So I suggest we should have a different place to put all the thread-related tests, and not running race condition check on them. What do you think?

64kramsystem commented 6 years ago

If you're completely sure that writing this code won't case the executable to panic (which is undesirable, even for broken code) due to pointers undefined behavior, then that's a solution to the problem.

st0012 commented 6 years ago

We can start writing the thread-related tests and I guess we'll find out then. But does this mean your mutex lock feature will be blocked?

64kramsystem commented 6 years ago

We can start writing the thread-related tests and I guess we'll find out then. But does this mean your mutex lock feature will be blocked?

I will check it!

64kramsystem commented 6 years ago

I've changed the title and description, since the race condition is not related to the test environment.