Skip to content

plugin: fix a double RLock bug#40588

Merged
thaJeztah merged 1 commit intomoby:masterfrom
sfzhu93:Double_RLock
Feb 27, 2020
Merged

plugin: fix a double RLock bug#40588
thaJeztah merged 1 commit intomoby:masterfrom
sfzhu93:Double_RLock

Conversation

@sfzhu93
Copy link
Copy Markdown

@sfzhu93 sfzhu93 commented Feb 27, 2020

- What I did
The following code has a double RLock bug:

moby/plugin/store.go

Lines 191 to 193 in 9fee52d

ps.RLock()
result = ps.getAllByCap(capability)
ps.RUnlock()

moby/plugin/store.go

Lines 75 to 78 in 9fee52d

func (ps *Store) getAllByCap(capability string) []plugingetter.CompatPlugin {
ps.RLock()
defer ps.RUnlock()

This PR deleted the outer RLock and RUnlock. They are redundant.

- How I did it
Removed line 191 and line 193.

- How to verify it
RLock followed by another RLock is buggy, due to Go's implementation of RLock. If another thread tries to acquire a Lock between the two RLock, then both threads will be blocked.

Thread 1 acquires the first RLock
Thread 2 blocks at acquiring Lock
Thread 1 blocks at acquiring the second RLock

- Description for the changelog
Removes a redundant RLock and RUnlock

- A picture of a cute animal (not mandatory but encouraged)
🐒

@lzhfromustc
plugin: fix a double RLock bug
34837fe 
Signed-off-by: Ziheng Liu <lzhfromustc@gmail.com>
@sfzhu93 sfzhu93 requested a review from cpuguy83 as a code owner February 27, 2020 01:11
cpuguy83
cpuguy83 approved these changes Feb 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah added this to the 20.03.0 milestone Feb 27, 2020
@thaJeztah thaJeztah added the kind/bugfix PR's that fix bugs label Feb 27, 2020
thaJeztah
thaJeztah approved these changes Feb 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 38f52c9 into moby:master Feb 27, 2020
@thaJeztah thaJeztah mentioned this pull request Jul 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

area/plugins kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

5 participants

@sfzhu93 @cpuguy83 @thaJeztah @AkihiroSuda @lzhfromustc