Skip to content
This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[17.07] cluster: Avoid recursive RLock#178

Merged
andrewhsu merged 1 commit into
docker-archive:17.07from
thaJeztah:17.07-backport-avoid-recursive-rlock
Aug 4, 2017
Merged

[17.07] cluster: Avoid recursive RLock#178
andrewhsu merged 1 commit into
docker-archive:17.07from
thaJeztah:17.07-backport-avoid-recursive-rlock

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

Cherry-picked from moby/moby@bd4f66c (moby/moby#34235)

no conflicts

ping @aaronlehmann @fcrisciani @tonistiigi

GetTasks can call GetService and GetNode with the read lock held. These
methods try to aquire the read side of the same lock. According to the
sync package documentation, this is not safe:

If a goroutine holds a RWMutex for reading, it must not expect this or
any other goroutine to be able to also take the read lock until the
first read lock is released. In particular, this prohibits recursive
read locking. This is to ensure that the lock eventually becomes
available; a blocked Lock call excludes new readers from acquiring the
lock.

Fix GetTasks to use the lower-level getService and getNode methods
instead. Also, use lockedManagerAction to simplify GetTasks.

GetTasks can call GetService and GetNode with the read lock held. These
methods try to aquire the read side of the same lock. According to the
sync package documentation, this is not safe:

> If a goroutine holds a RWMutex for reading, it must not expect this or
> any other goroutine to be able to also take the read lock until the
> first read lock is released. In particular, this prohibits recursive
> read locking. This is to ensure that the lock eventually becomes
> available; a blocked Lock call excludes new readers from acquiring the
> lock.

Fix GetTasks to use the lower-level getService and getNode methods
instead. Also, use lockedManagerAction to simplify GetTasks.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
(cherry picked from commit bd4f66c)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@tonistiigi
Copy link
Copy Markdown
Contributor

LGTM

@seemethere
Copy link
Copy Markdown
Contributor

seemethere commented Aug 4, 2017

Test failures are known as per #140:

Checked with this script: https://gist.github.com/seemethere/6128cb2391cd43b65b0ff4880af427e5

@andrewhsu andrewhsu added this to the 17.07.0 milestone Aug 4, 2017
Copy link
Copy Markdown
Contributor

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewhsu andrewhsu merged commit 8cc9393 into docker-archive:17.07 Aug 4, 2017
@thaJeztah thaJeztah deleted the 17.07-backport-avoid-recursive-rlock branch August 4, 2017 23:07
docker-jenkins pushed a commit that referenced this pull request Sep 4, 2018
[master] bump Go to 1.10.4
Upstream-commit: 93bf7f2
Component: packaging
docker-jenkins pushed a commit that referenced this pull request Mar 28, 2019
[18.09 backport] Use original process spec for execs
Upstream-commit: b4bf217633d7884999b675b8a7b9b81892b46086
Component: engine
silvin-lubecki pushed a commit to silvin-lubecki/docker-ce that referenced this pull request Feb 3, 2020
…void-recursive-rlock

[17.07] cluster: Avoid recursive RLock
akrasnov-drv pushed a commit to drivenets/docker-ce that referenced this pull request Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants