Skip to content

fix race conditions on iterators #89

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
Merged
merged 1 commit into from
Oct 17, 2019
Merged

fix race conditions on iterators #89

merged 1 commit into from
Oct 17, 2019

Conversation

lwsanty
Copy link
Contributor

@lwsanty lwsanty commented Oct 15, 2019 •
edited
Loading

during gitbase tests execution on the binary built with race detection flag the race was detected

================== WARNING: DATA RACE Read at 0x00c000cf43e0 by goroutine 22: github.com/src-d/go-borges/libraries.(*mergedRepoIter).Close() /home/lwsanty/goproj/lwsanty/go-borges/libraries/iterator.go:57 +0x3e github.com/src-d/gitbase.(*repositoryPartitionIter).Close() /home/lwsanty/goproj/lwsanty/gitbase/partition.go:96 +0x7b github.com/src-d/go-mysql-server/sql/plan.(*exchangeRowIter).Close() /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/exchange.go:289 +0xca github.com/src-d/go-mysql-server/sql/plan.(*limitIter).Close() /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/limit.go:77 +0x55 github.com/src-d/go-mysql-server/sql.(*spanIter).Close() /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/session.go:416 +0x71 github.com/src-d/go-mysql-server/sql/plan.(*trackedRowIter).Close() /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/process.go:142 +0x73 github.com/src-d/go-mysql-server/server.(*Handler).ComQuery() /home/lwsanty/goproj/lwsanty/go-mysql-server/server/handler.go:278 +0xfa7 vitess.io/vitess/go/mysql.(*Conn).execQuery() /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/conn.go:819 +0x20b vitess.io/vitess/go/mysql.(*Conn).handleNextCommand() /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/conn.go:749 +0x16f8 vitess.io/vitess/go/mysql.(*Listener).handle() /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/server.go:441 +0x1446 Previous write at 0x00c000cf43e0 by goroutine 28: github.com/src-d/go-borges/libraries.(*mergedRepoIter).Next() /home/lwsanty/goproj/lwsanty/go-borges/libraries/iterator.go:37 +0x1b0 github.com/src-d/gitbase.(*repositoryPartitionIter).Next() /home/lwsanty/goproj/lwsanty/gitbase/partition.go:79 +0x7e github.com/src-d/go-mysql-server/sql/plan.(*exchangeRowIter).iterPartitions() /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/exchange.go:195 +0x292 Goroutine 22 (running) created at: vitess.io/vitess/go/mysql.(*Listener).Accept() /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/server.go:243 +0x188 github.com/src-d/gitbase/cmd/gitbase/command.(*Server).Execute() /home/lwsanty/goproj/lwsanty/go-mysql-server/server/server.go:79 +0xc8d github.com/jessevdk/go-flags.(*Parser).ParseArgs() /home/lwsanty/goproj/gopath/pkg/mod/github.com/jessevdk/go-flags@v1.4.0/parser.go:316 +0xf57 main.main() /home/lwsanty/goproj/gopath/pkg/mod/github.com/jessevdk/go-flags@v1.4.0/parser.go:186 +0x8a3 Goroutine 28 (finished) created at: github.com/src-d/go-mysql-server/sql/plan.(*exchangeRowIter).start() /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/exchange.go:143 +0xa0 ==================

this PR provides fix to the current race

related to src-d/gitbase#955

Signed-off-by: lwsanty lwsanty@gmail.com
This was referenced Oct 16, 2019
Closed
Merged
@jfontan jfontan requested a review from mcarmonaa October 16, 2019 13:33
jfontan
jfontan suggested changes Oct 16, 2019
View reviewed changes
libraries/iterator.go Outdated
Comment on lines 92 to 94
i.mu.Lock()
i.iters = i.iters[1:]
i.mu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it protect the whole function so the rest of hash accesses are in the lock? These:

if len(i.iters) == 0 { loc, err := i.iters[0].Next()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you are right, I just narrowed locks to fix race, but the whole method should be locked
libraries/iterator.go Outdated
Comment on lines 146 to 148
i.mu.Lock()
i.iters = i.iters[1:]
i.mu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with mergedLocationIter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done
@lwsanty lwsanty force-pushed the fix-race-conditions branch from 4159f68 to 2584502 Compare October 16, 2019 14:41
@lwsanty lwsanty requested a review from jfontan October 16, 2019 14:42
jfontan
jfontan suggested changes Oct 16, 2019
View reviewed changes
libraries/iterator.go Outdated
return nil, err
}

i.mu.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be changed to lock the whole function.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

locking here causes locking while debugging via gitbase
Probably, some additional changes need to be done here

goroutine 9 [semacquire]: sync.runtime_SemacquireMutex(0xc000cf98e4, 0xc000cfd000, 0x1) /usr/local/go/src/runtime/sema.go:71 +0x47 sync.(*Mutex).lockSlow(0xc000cf98e0) /usr/local/go/src/sync/mutex.go:138 +0xfc sync.(*Mutex).Lock(...) /usr/local/go/src/sync/mutex.go:81 github.com/src-d/go-borges/libraries.(*mergedRepoIter).Next(0xc000cf98e0, 0x0, 0x0, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/go-borges/libraries/iterator.go:29 +0x256 github.com/src-d/go-borges/libraries.(*mergedRepoIter).Next(0xc000cf98e0, 0x0, 0x0, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/go-borges/libraries/iterator.go:42 +0x17b github.com/src-d/gitbase.(*RepositoryIter).Next(0xc000cf9900, 0xc000cfaf00, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/gitbase/repository_pool.go:133 +0x38 github.com/src-d/gitbase.partitioned.PartitionCount(0xc000341220, 0x12e4720, 0x11d8b01, 0x7f69f9179d78) /home/lwsanty/goproj/lwsanty/gitbase/partition.go:32 +0x91 github.com/src-d/go-mysql-server/sql/analyzer.trackProcess.func1(0x17f4e20, 0xc000cf51b0, 0x0, 0x0, 0x0, 0xc000cf5700) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/analyzer/process.go:37 +0x473 github.com/src-d/go-mysql-server/sql/plan.TransformUp(0x17f4e20, 0xc000cf51b0, 0xc000ccf1d0, 0xc000cf57c0, 0x0, 0xc000cf5700, 0xc000ccf000) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/transform.go:17 +0x269 github.com/src-d/go-mysql-server/sql/plan.TransformUp(0x17f48e0, 0xc000cf96c0, 0xc000ccf1d0, 0xc000cf57a0, 0x0, 0xc000cf5700, 0xc000ccf078) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/transform.go:22 +0xfe github.com/src-d/go-mysql-server/sql/plan.TransformUp(0x17f4d60, 0xc000cfccf0, 0xc000ccf1d0, 0xc000cf5780, 0x0, 0xc000cf5700, 0xc000ccf0f0) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/transform.go:22 +0xfe github.com/src-d/go-mysql-server/sql/plan.TransformUp(0x17f4d60, 0xc000cfcd20, 0xc000ccf1d0, 0xc000cf5760, 0x0, 0xc000cf8500, 0x2) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/transform.go:22 +0xfe github.com/src-d/go-mysql-server/sql/plan.TransformUp(0x17f4b80, 0xc000cf96e0, 0xc000ccf1d0, 0x0, 0xc000cf5700, 0x17c5500, 0x9d7e52) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/plan/transform.go:22 +0xfe github.com/src-d/go-mysql-server/sql/analyzer.trackProcess(0xc000341220, 0xc000c28cc0, 0x17f4b80, 0xc000cf96e0, 0x17f4b80, 0xc000cf96e0, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/analyzer/process.go:22 +0x16b github.com/src-d/go-mysql-server/sql/analyzer.(*Batch).evalOnce(0xc000c28c90, 0xc000341220, 0xc000c28cc0, 0x17f4b80, 0xc000cf96e0, 0x17f4b80, 0xc000cf96e0, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/analyzer/batch.go:67 +0x98 github.com/src-d/go-mysql-server/sql/analyzer.(*Batch).Eval(0xc000c28c90, 0xc000341220, 0xc000c28cc0, 0x17f4b80, 0xc000cf96e0, 0x17f4b80, 0xc000cf96e0, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/analyzer/batch.go:38 +0x7f github.com/src-d/go-mysql-server/sql/analyzer.(*Analyzer).Analyze(0xc000c28cc0, 0xc000341220, 0x17f4b80, 0xc000ca6440, 0x0, 0x0, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/go-mysql-server/sql/analyzer/analyzer.go:171 +0x275 github.com/src-d/go-mysql-server.(*Engine).Query(0xc0003bbd40, 0xc0003411d0, 0xc0000d6120, 0x8f, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...) /home/lwsanty/goproj/lwsanty/go-mysql-server/engine.go:143 +0x2d7 github.com/src-d/go-mysql-server/server.(*Handler).ComQuery(0xc000050400, 0xc000b2fa00, 0xc0000d6120, 0x8f, 0xc0002de880, 0x0, 0x0) /home/lwsanty/goproj/lwsanty/go-mysql-server/server/handler.go:134 +0x2de vitess.io/vitess/go/mysql.(*Conn).execQuery(0xc000b2fa00, 0xc0000d6120, 0x8f, 0x17eb440, 0xc000050400, 0x0, 0xc000496000, 0x7f69f929f008) /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/conn.go:819 +0x118 vitess.io/vitess/go/mysql.(*Conn).handleNextCommand(0xc000b2fa00, 0x17eb440, 0xc000050400, 0xea89f, 0x3ba10494) /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/conn.go:749 +0x12fe vitess.io/vitess/go/mysql.(*Listener).handle(0xc000132580, 0x17fd580, 0xc00019caa0, 0xc000000001, 0xbf61efe85b573e40, 0x3ba10494, 0x256a0e0) /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/server.go:441 +0xd3f created by vitess.io/vitess/go/mysql.(*Listener).Accept /home/lwsanty/goproj/gopath/pkg/mod/vitess.io/vitess@v3.0.0-rc.3.0.20190602171040-12bfde34629c+incompatible/go/mysql/server.go:243 +0xf2
@lwsanty lwsanty requested a review from jfontan October 17, 2019 11:45
@lwsanty
fix race conditions on iterators
1c84129 
Signed-off-by: lwsanty <lwsanty@gmail.com>
@lwsanty lwsanty force-pushed the fix-race-conditions branch from 2584502 to 1c84129 Compare October 17, 2019 13:10
@jfontan jfontan merged commit c26ddd9 into src-d:master Oct 17, 2019
lwsanty added a commit to src-d/gitbase that referenced this pull request Oct 17, 2019
@lwsanty
update go-borges dependency
aa62244 
contains: 1) non-recursive iterators 2) race conditions fix src-d/go-borges#89 Signed-off-by: lwsanty <lwsanty@gmail.com>
@lwsanty lwsanty mentioned this pull request Oct 17, 2019
Merged
lwsanty added a commit to src-d/gitbase that referenced this pull request Oct 17, 2019
@lwsanty
update go-borges dependency
02a0182 
contains: 1) non-recursive iterators 2) race conditions fix src-d/go-borges#89 Signed-off-by: lwsanty <lwsanty@gmail.com>
lwsanty added a commit to src-d/gitbase that referenced this pull request Oct 17, 2019
@lwsanty
update go-borges dependency
4319bdb 
contains: 1) non-recursive iterators 2) race conditions fix src-d/go-borges#89 Signed-off-by: lwsanty <lwsanty@gmail.com>
lwsanty added a commit to src-d/gitbase that referenced this pull request Oct 17, 2019
@lwsanty
update go-borges dependency
f2328ca 
contains: 1) non-recursive iterators 2) race conditions fix src-d/go-borges#89 Signed-off-by: lwsanty <lwsanty@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@lwsanty @jfontan @mcarmonaa