store: thread safe for Register kv.Driver by w169q169 · Pull Request #28267 · pingcap/tidb

w169q169 · 2021-09-22T09:46:04Z

What problem does this PR solve?

Issue Number: close #28318

introduced in #27993

Problem Summary:

What is changed and how it works?

What's Changed:
Register function is not thread safe.

tidb/store/store.go

Lines 31 to 40 in e60f20b

    
           func Register(name string, driver kv.Driver) error { 
        
           	name = strings.ToLower(name) 
        
           	if _, ok := stores[name]; ok { 
        
           		return errors.Errorf("%s is already registered", name) 
        
           	} 
        
           	stores[name] = driver 
        
           	return nil 
        
           }

Run those parallel test functions will suffer fatal error for concurrent map writes on occasionally.

tidb/store/store_test.go

Lines 765 to 770 in e60f20b

    
           func TestRetryOpenStore(t *testing.T) { 
        
           	t.Parallel() 
        
           	begin := time.Now() 
        
           	require.NoError(t, Register("dummy", &brokenStore{})) 
        
           	store, err := newStoreWithRetry("dummy://dummy-store", 3) 
        
           	if store != nil {

tidb/store/store_test.go

Lines 780 to 785 in e60f20b

    
           func TestOpenStore(t *testing.T) { 
        
           	t.Parallel() 
        
           	require.NoError(t, Register("open", &brokenStore{})) 
        
           	store, err := newStoreWithRetry(":", 3) 
        
           	if store != nil { 
        
           		defer func() {

How it Works:
Add mutex Lock for map variable in package write and read.

Release note

None

ti-chi-bot · 2021-09-22T09:46:05Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

morgo
tisonkun

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

w169q169 · 2021-09-23T09:06:14Z

/cc @tisonkun @morgo
PTAL

tisonkun

I'd prefer move the two tests touching "Register" in serial tests.

I'm unsure whether or not the function should be design as thread safe or with assumption only call in serial. But if you can elaborate the reason, I can agree with this change.

tisonkun · 2021-09-23T09:13:53Z

Also you can report an unstable test in #25899 (if you don't have write permission, create an unstable test issue and notify me) and let this PR fixes it.

w169q169 · 2021-09-23T09:46:25Z

I'd prefer move the two tests touching "Register" in serial tests.

I'm unsure whether or not the function should be design as thread safe or with assumption only call in serial. But if you can elaborate the reason, I can agree with this change.

moving the tests using Register in serial tests is quit a litter easy to fix the problem.
Register only run parallel in test scenes, howerver if people use the function not too carefully it will cause a thread error that is unstable and unnotice.So we can use the defensesive code (suppose that people always use wrong way ) to avoid the error will happen in the future , even on current the function is used right in most scenes.
if you have different idea, please let me know and I will fix the test according to your opinion.

tisonkun

You convince me that this function is not in a significant path and we can use defensive code trade off a bit performance.

@morgo please give a final check.

... also, I'm curious that Golang doesn't have a concurrent map out-of-the-box?

tisonkun · 2021-09-23T10:07:11Z

store/store.go

+
+	storesLock.RLock()
+
 	d, ok := stores[name]


Move two storesLock.RUnlock() right after this line?

... or write a function to retrieve the driver, like:

func loadDriver(name string) (kv.Driver, bool) { storesLock.RUnlock() defer storesLock.Unlock() d, ok := stores[name] return d, ok }

I'd prefer write a function to get the driver , and scope for lock is more less in this way

tisonkun

COMMENT DELETED. Only comments above that we can narrow or factor out the steps to retrieve the driver.

tisonkun

LGTM.

tisonkun · 2021-09-24T01:45:32Z

/cc @morgo @disksing

PTAL.

disksing · 2021-09-24T09:21:32Z

/cc @morgo @disksing

PTAL.

LGTM. Defer to @morgo for another check.

tisonkun · 2021-09-24T23:46:40Z

@morgo this unstable test occurs several times in these days. Could you please do a final check on this fix?

morgo · 2021-09-25T00:56:56Z

/merge

ti-chi-bot · 2021-09-25T00:56:59Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 311ce77

ti-chi-bot · 2021-09-25T00:57:10Z

@w169q169: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

morgo · 2021-09-25T00:57:40Z

Thank you for the contribution!

store: thread safe for Register kv.Driver

17b97e9

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 22, 2021

Merge branch 'master' into issue-27993

b40c248

ti-chi-bot requested review from morgo and tisonkun September 23, 2021 09:06

tisonkun reviewed Sep 23, 2021

View reviewed changes

tisonkun approved these changes Sep 23, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 23, 2021

tisonkun reviewed Sep 23, 2021

View reviewed changes

tisonkun requested changes Sep 23, 2021

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 23, 2021

fix for review

311ce77

w169q169 requested a review from tisonkun September 24, 2021 01:12

tisonkun approved these changes Sep 24, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 24, 2021

ti-chi-bot requested a review from disksing September 24, 2021 01:45

tisonkun mentioned this pull request Sep 24, 2021

Unstable test store_test.go fatal error: concurrent map writes #28318

Closed

morgo approved these changes Sep 25, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 25, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 25, 2021

Merge branch 'master' into issue-27993

ae8c3c6

ti-chi-bot merged commit 6a8375a into pingcap:master Sep 25, 2021

	func Register(name string, driver kv.Driver) error {
	name = strings.ToLower(name)

	if _, ok := stores[name]; ok {
	return errors.Errorf("%s is already registered", name)
	}

	stores[name] = driver
	return nil
	}

	func TestRetryOpenStore(t *testing.T) {
	t.Parallel()
	begin := time.Now()
	require.NoError(t, Register("dummy", &brokenStore{}))
	store, err := newStoreWithRetry("dummy://dummy-store", 3)
	if store != nil {

	func TestOpenStore(t *testing.T) {
	t.Parallel()
	require.NoError(t, Register("open", &brokenStore{}))
	store, err := newStoreWithRetry(":", 3)
	if store != nil {
	defer func() {

Conversation

w169q169 commented Sep 22, 2021 • edited by tisonkun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Release note

Uh oh!

ti-chi-bot commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w169q169 commented Sep 23, 2021

Uh oh!

tisonkun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Sep 23, 2021

Uh oh!

w169q169 commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

tisonkun Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

w169q169 Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

tisonkun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Sep 24, 2021

Uh oh!

disksing commented Sep 24, 2021

Uh oh!

tisonkun commented Sep 24, 2021

Uh oh!

morgo commented Sep 25, 2021

Uh oh!

ti-chi-bot commented Sep 25, 2021

Uh oh!

ti-chi-bot commented Sep 25, 2021

Uh oh!

morgo commented Sep 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

w169q169 commented Sep 22, 2021 •

edited by tisonkun

Loading

ti-chi-bot commented Sep 22, 2021 •

edited

Loading

tisonkun left a comment •

edited

Loading

w169q169 commented Sep 23, 2021 •

edited

Loading

tisonkun left a comment •

edited

Loading