Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Concurrent access to variable causes a race condition detection #499

Open
64kramsystem opened this issue Nov 2, 2017 · 10 comments
Open

Concurrent access to variable causes a race condition detection #499

64kramsystem opened this issue Nov 2, 2017 · 10 comments
Labels
bug Priority High VM
Milestone
version 0.2.0

Comments

@64kramsystem
Copy link
Member

64kramsystem commented Nov 2, 2017 •
edited

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
@64kramsystem 64kramsystem mentioned this issue Nov 2, 2017
Merged
@st0012
Copy link
Member

st0012 commented Nov 2, 2017

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

@st0012
Copy link
Member

st0012 commented Nov 7, 2017

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
Copy link
Member

st0012 commented Nov 9, 2017

@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
Copy link
Member Author

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

@64kramsystem
Copy link
Member Author

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

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
Copy link
Member

st0012 commented Nov 14, 2017

@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
Copy link
Member Author

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
Copy link
Member

st0012 commented Nov 14, 2017

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
Copy link
Member Author

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 64kramsystem changed the title Testing threads causes a race condition Concurrent access to variable causes a race condition Nov 16, 2017
@64kramsystem 64kramsystem changed the title Concurrent access to variable causes a race condition Concurrent access to variable causes a race condition detection Nov 16, 2017
@64kramsystem
Copy link
Member Author

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

@st0012 st0012 added this to the version 0.2.0 milestone Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Priority High VM
Projects
None yet
Milestone
version 0.2.0
Development

No branches or pull requests
2 participants
@64kramsystem @st0012