Skip to content

Commit

Permalink
cluster: Avoid recursive RLock
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
aaronlehmann committed Jul 26, 2017
1 parent 40876b1 commit d83c601
Showing 1 changed file with 31 additions and 38 deletions.
69 changes: 31 additions & 38 deletions daemon/cluster/tasks.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,52 +11,45 @@ import (

// GetTasks returns a list of tasks matching the filter options.
func (c *Cluster) GetTasks(options apitypes.TaskListOptions) ([]types.Task, error) {
c.mu.RLock()
defer c.mu.RUnlock()
var r *swarmapi.ListTasksResponse

state := c.currentNodeState()
if !state.IsActiveManager() {
return nil, c.errNoManager(state)
}

byName := func(filter filters.Args) error {
if filter.Include("service") {
serviceFilters := filter.Get("service")
for _, serviceFilter := range serviceFilters {
service, err := c.GetService(serviceFilter, false)
if err != nil {
return err
if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
byName := func(filter filters.Args) error {
if filter.Include("service") {
serviceFilters := filter.Get("service")
for _, serviceFilter := range serviceFilters {
service, err := getService(ctx, state.controlClient, serviceFilter, false)
if err != nil {
return err
}
filter.Del("service", serviceFilter)
filter.Add("service", service.ID)
}
filter.Del("service", serviceFilter)
filter.Add("service", service.ID)
}
}
if filter.Include("node") {
nodeFilters := filter.Get("node")
for _, nodeFilter := range nodeFilters {
node, err := c.GetNode(nodeFilter)
if err != nil {
return err
if filter.Include("node") {
nodeFilters := filter.Get("node")
for _, nodeFilter := range nodeFilters {
node, err := getNode(ctx, state.controlClient, nodeFilter)
if err != nil {
return err
}
filter.Del("node", nodeFilter)
filter.Add("node", node.ID)
}
filter.Del("node", nodeFilter)
filter.Add("node", node.ID)
}
return nil
}
return nil
}

filters, err := newListTasksFilters(options.Filters, byName)
if err != nil {
return nil, err
}

ctx, cancel := c.getRequestContext()
defer cancel()
filters, err := newListTasksFilters(options.Filters, byName)
if err != nil {
return err
}

r, err := state.controlClient.ListTasks(
ctx,
&swarmapi.ListTasksRequest{Filters: filters})
if err != nil {
r, err = state.controlClient.ListTasks(
ctx,
&swarmapi.ListTasksRequest{Filters: filters})
return err
}); err != nil {
return nil, err
}

Expand Down

0 comments on commit d83c601

Please sign in to comment.