Implement R/W Lock #493

64kramsystem · 2017-10-30T22:42:18Z

Implementation of Readers/writer lock.

In order to allow execution of some tests with race detector, this PR adds support for the noracedetection tag, and separates the test build into regular, and noracedetection.

Closes #477

codecov · 2017-10-31T00:52:39Z

Codecov Report

Merging #493 into master will increase coverage by 0.31%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
+ Coverage   83.33%   83.64%   +0.31%     
==========================================
  Files          52       53       +1     
  Lines        9176     9308     +132     
==========================================
+ Hits         7647     7786     +139     
+ Misses       1291     1284       -7     
  Partials      238      238

Impacted Files	Coverage Δ
vm/vm.go	`86.25% <ø> (ø)`	⬆️
vm/concurrent_rw_lock.go	`95.45% <95.45%> (ø)`
vm/class.go	`86.67% <0%> (+1.44%)`	⬆️
vm/error.go	`83.78% <0%> (+10.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98fe26c...6a5065c. Read the comment docs.

64kramsystem · 2017-11-01T23:16:40Z

@st0012 a couple of notes about this PR:

it's the basic version; I will add the methods (acquire|release)_*_lock in a second iteration
the UTs can be made much more efficient when merging Add float support to sleep() #498; right now, I have to use integer sleeping times; with the mentioned PR, I could use fractions of seconds.
as far as I've read in the documentation, Goby's current thread implementation doesn't allow to wait for a thread to terminate, so I have to artificially introduce another sleep.

64kramsystem · 2017-11-01T23:22:00Z

@st0012 do you have any insights about the build failure (link)? the tests I wrote pass when I execute them locally, but when running the full build, it seems go is detecting threading issues (is it the testing framework?):

WARNING: DATA RACE
Read at 0x00c420582460 by goroutine 106:
  github.com/goby-lang/goby/vm.(*thread).hasError()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:95 +0x9e
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:51 +0x67b
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).builtinMethodYield()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:140 +0x4c4
  github.com/goby-lang/goby/vm.builtinConcurrentRWLockInstanceMethods.func1.1()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/concurrent_rw_lock.go:80 +0xe2
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:63 +0x356
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).evalBuiltinMethod()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:240 +0x66d
  github.com/goby-lang/goby/vm.glob..func25()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/instruction.go:487 +0x7a3
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:124 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:50 +0x66a
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).builtinMethodYield()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:140 +0x4c4
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func17.1.1()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/class.go:984 +0x60
Previous write at 0x00c420582460 by goroutine 107:
  github.com/goby-lang/goby/vm.(*baseFrame).insertLCL()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/call_frame.go:120 +0x85
  github.com/goby-lang/goby/vm.glob..func9()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/instruction.go:165 +0x253
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:124 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:50 +0x66a
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).builtinMethodYield()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:140 +0x4c4
  github.com/goby-lang/goby/vm.builtinConcurrentRWLockInstanceMethods.func1.1()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/concurrent_rw_lock.go:80 +0xe2
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:63 +0x356
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).evalBuiltinMethod()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:240 +0x66d
  github.com/goby-lang/goby/vm.glob..func25()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/instruction.go:487 +0x7a3
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:124 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:50 +0x66a
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).builtinMethodYield()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:140 +0x4c4
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func17.1.1()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/class.go:984 +0x60
Goroutine 106 (running) created at:
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func17.1()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/class.go:983 +0x3fa
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:63 +0x356
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).evalBuiltinMethod()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:240 +0x66d
  github.com/goby-lang/goby/vm.glob..func25()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/instruction.go:487 +0x7a3
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:124 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:50 +0x66a
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*VM).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/vm.go:270 +0x4e
  github.com/goby-lang/goby/vm.(*VM).ExecInstructions()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/vm.go:166 +0xc98
  github.com/goby-lang/goby/vm.(*VM).testEval()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/vm_test.go:185 +0xc2
  github.com/goby-lang/goby/vm.TestRWLockWithReadLockSharedRead()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/concurrent_rw_lock_test.go:40 +0x81
  testing.tRunner()
      /home/travis/.gimme/versions/go1.9.2.linux.amd64/src/testing/testing.go:746 +0x16c
Goroutine 107 (finished) created at:
  github.com/goby-lang/goby/vm.builtinClassCommonInstanceMethods.func17.1()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/class.go:983 +0x3fa
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:63 +0x356
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*thread).evalBuiltinMethod()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:240 +0x66d
  github.com/goby-lang/goby/vm.glob..func25()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/instruction.go:487 +0x7a3
  github.com/goby-lang/goby/vm.(*thread).execInstruction()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:124 +0x107
  github.com/goby-lang/goby/vm.(*thread).evalCallFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:50 +0x66a
  github.com/goby-lang/goby/vm.(*thread).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/thread.go:42 +0xa6
  github.com/goby-lang/goby/vm.(*VM).startFromTopFrame()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/vm.go:270 +0x4e
  github.com/goby-lang/goby/vm.(*VM).ExecInstructions()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/vm.go:166 +0xc98
  github.com/goby-lang/goby/vm.(*VM).testEval()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/vm_test.go:185 +0xc2
  github.com/goby-lang/goby/vm.TestRWLockWithReadLockSharedRead()
      /home/travis/gopath/src/github.com/goby-lang/goby/vm/concurrent_rw_lock_test.go:40 +0x81
  testing.tRunner()
      /home/travis/.gimme/versions/go1.9.2.linux.amd64/src/testing/testing.go:746 +0x16c

st0012 · 2017-11-02T08:50:46Z

@saveriomiroddi Looks like it is because multiple threads want to check if there's error on the stack, but they all want to access the same stack (I guess it's the main stack) instead of their own stack.
I can't really tell where goes wrong because I don't have time to play with it yet, but you can reproduce this locally by running test with go test ./vm --run TestRWLockWithReadLockSharedRead -race

64kramsystem · 2017-11-02T09:15:29Z

@st0012 I've opened an issue (#499); the minimal test case that triggers the error doesn't involve Concurrent::RWLock:

code := `
finish_message = nil

thread do
  finish_message = "thread"
end

sleep 1
finish_message
`

v := initTestVM()
v.testEval(t, code, getFilename())

so I suppose there's a problem either with the testing framework, or the interpreter. I definitely think that threading needs unit testing (especially since it's a selling point of Goby), also to find out if the testing framework is adequate, or needs to be improved.

64kramsystem · 2017-11-15T11:14:34Z

@st0012 following up on #499, I had a look at the travis configuraiton; do you want to adopt a convention for not testing race conditions on some UTs? For example, all those including Threaded in the UT name should not be tested with the race detector?

st0012 · 2017-11-16T06:52:05Z

@saveriomiroddi This is what I'm concerning about. If there really are multiple threads running in a test case, the race condition will very likely to happen. So in this case, use race condition detector doesn't make sense. However, it'll be hard to find out some cases that race condition might cause nil pointer error.

64kramsystem · 2017-11-16T17:45:05Z

@st0012 The build tag strategy didn't work for filtering the tests to be performed without race detector (because untagged test suites are always run). I also had a fair amount of hair pulling because I didn't understand how the travis test (plugin) filtering was working - turns out, it was broken (see #518).
Therefore, I've used the strategy used in #519 (environment variable+UT naming convention).

64kramsystem · 2017-11-16T17:45:26Z

This PR builds on top of #519, so I'm marking as blocked by it.

Blocked by #519.

64kramsystem · 2017-11-17T10:14:35Z

Removed the Blocked status label. Build is now green.

64kramsystem · 2017-11-22T17:58:34Z

ok, I've fixed the failures from the master merge; once the build is green, it's ready for merge.

64kramsystem self-assigned this Oct 30, 2017

64kramsystem force-pushed the sm-implement_rw_mutex branch from 3fb0a39 to c501cdb Compare November 1, 2017 23:13

ghost added the in progress label Nov 1, 2017

64kramsystem requested a review from st0012 November 1, 2017 23:14

64kramsystem changed the title ~~[WIP] Implement R/W Lock~~ Implement R/W Lock Nov 1, 2017

64kramsystem force-pushed the sm-implement_rw_mutex branch from c501cdb to 1e9752b Compare November 1, 2017 23:56

64kramsystem mentioned this pull request Nov 2, 2017

Concurrent access to variable causes a race condition detection #499

Open

st0012 added blocked Feature Library Priority Medium VM labels Nov 10, 2017

64kramsystem force-pushed the sm-implement_rw_mutex branch 6 times, most recently from 08ad823 to 512a44f Compare November 16, 2017 12:15

64kramsystem removed the blocked label Nov 16, 2017

64kramsystem changed the title ~~Implement R/W Lock~~ WIP: Implement R/W Lock Nov 16, 2017

64kramsystem force-pushed the sm-implement_rw_mutex branch 3 times, most recently from 98d9577 to c65cd83 Compare November 16, 2017 17:04

64kramsystem force-pushed the sm-implement_rw_mutex branch 2 times, most recently from 76c06a5 to 6b639ea Compare November 16, 2017 17:44

64kramsystem added the blocked label Nov 16, 2017

64kramsystem changed the title ~~WIP: Implement R/W Lock~~ Implement R/W Lock Nov 16, 2017

64kramsystem force-pushed the sm-implement_rw_mutex branch from 6b639ea to 2bbc8a8 Compare November 17, 2017 10:10

64kramsystem removed the blocked label Nov 17, 2017

64kramsystem mentioned this pull request Nov 17, 2017

Add float support to sleep() #498

Merged

64kramsystem added 2 commits November 17, 2017 18:26

Basic implementation of Readers-writer lock (Concurrent::RWLock)

4f3e089

Add RWLock#(acquire|release)_(read|write)_lock methods

d49e6e9

64kramsystem force-pushed the sm-implement_rw_mutex branch from 2bbc8a8 to d49e6e9 Compare November 17, 2017 17:27

st0012 approved these changes Nov 22, 2017

View reviewed changes

64kramsystem added 2 commits November 22, 2017 16:13

Merge branch 'master' into sm-implement_rw_mutex

1d0c781

Fixed failures from master merge

6a5065c

st0012 merged commit d1390cc into goby-lang:master Nov 23, 2017

64kramsystem deleted the sm-implement_rw_mutex branch January 10, 2018 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement R/W Lock #493

Implement R/W Lock #493

64kramsystem commented Oct 30, 2017 •

edited

Loading

codecov bot commented Oct 31, 2017 •

edited

Loading

64kramsystem commented Nov 1, 2017 •

edited

Loading

64kramsystem commented Nov 1, 2017

st0012 commented Nov 2, 2017 •

edited

Loading

64kramsystem commented Nov 2, 2017 •

edited

Loading

64kramsystem commented Nov 15, 2017

st0012 commented Nov 16, 2017

64kramsystem commented Nov 16, 2017

64kramsystem commented Nov 16, 2017

64kramsystem commented Nov 17, 2017

64kramsystem commented Nov 22, 2017

Implement R/W Lock #493

Implement R/W Lock #493

Conversation

64kramsystem commented Oct 30, 2017 • edited Loading

codecov bot commented Oct 31, 2017 • edited Loading

Codecov Report

64kramsystem commented Nov 1, 2017 • edited Loading

64kramsystem commented Nov 1, 2017

st0012 commented Nov 2, 2017 • edited Loading

64kramsystem commented Nov 2, 2017 • edited Loading

64kramsystem commented Nov 15, 2017

st0012 commented Nov 16, 2017

64kramsystem commented Nov 16, 2017

64kramsystem commented Nov 16, 2017

64kramsystem commented Nov 17, 2017

64kramsystem commented Nov 22, 2017

64kramsystem commented Oct 30, 2017 •

edited

Loading

codecov bot commented Oct 31, 2017 •

edited

Loading

64kramsystem commented Nov 1, 2017 •

edited

Loading

st0012 commented Nov 2, 2017 •

edited

Loading

64kramsystem commented Nov 2, 2017 •

edited

Loading