Fix race conditions #6118

Merged
merged 6 commits into from May 30, 2014

Conversation

Projects
None yet
4 participants
@LK4D4
Contributor

LK4D4 commented May 30, 2014

I've built docker with race flag and found some race condition. After fixes, with 15 minutes load of ./docker-stress -c 20 -t 1 I didn't saw any races.

LK4D4 added some commits May 29, 2014

Fix races on TagStore accessing
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
Fix race on shutting down
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
Fix races in set/get currentInterfaces in networkdriver
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
Fix race in native driver on activeContainers usage
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
@LK4D4

This comment has been minimized.

Show comment
Hide comment

LK4D4 added some commits May 30, 2014

Goroutine-safe daemon.containers
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
Atomically increment sequence in pkg/netlink
Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
@pnasrat

View changes

pkg/safelist/safelist.go
+ }
+}
+
+func (l *List) Back() *list.Element {

This comment has been minimized.

@pnasrat

pnasrat May 30, 2014

Contributor

Please add comments for godoc for each of the exported functions.

@pnasrat

pnasrat May 30, 2014

Contributor

Please add comments for godoc for each of the exported functions.

@pnasrat

View changes

pkg/safelist/safelist.go
+func (l *List) Back() *list.Element {
+ l.mu.Lock()
+ res := l.List.Back()
+ l.mu.Unlock()

This comment has been minimized.

@pnasrat

pnasrat May 30, 2014

Contributor

defer Unlock might be preferred style

@pnasrat

pnasrat May 30, 2014

Contributor

defer Unlock might be preferred style

@pnasrat

View changes

pkg/safelist/safelist.go
+ return res
+}
+
+// Prev needed just for synchronization of elements

This comment has been minimized.

@pnasrat

pnasrat May 30, 2014

Contributor

If these are just needed internally don't export. Else the godoc should be user facing.

@pnasrat

pnasrat May 30, 2014

Contributor

If these are just needed internally don't export. Else the godoc should be user facing.

@pnasrat

View changes

pkg/safelist/safelist.go
+ "sync"
+)
+
+// Package safelist containing goroutine-safe linked list

This comment has been minimized.

@pnasrat

pnasrat May 30, 2014

Contributor

Probably should have some tests for this package.

@pnasrat

pnasrat May 30, 2014

Contributor

Probably should have some tests for this package.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 May 30, 2014

Contributor

@pnasrat sorry, I've rewrited it already without using ugly safelist :) I'll push it soon.
But defer brings big overhead comparing to light functions. I wrote about it here

Contributor

LK4D4 commented May 30, 2014

@pnasrat sorry, I've rewrited it already without using ugly safelist :) I'll push it soon.
But defer brings big overhead comparing to light functions. I wrote about it here

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 30, 2014

Contributor

@LK4D4 is this ready for review? I didn't understand your last comment.

Contributor

crosbymichael commented May 30, 2014

@LK4D4 is this ready for review? I didn't understand your last comment.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 May 30, 2014

Contributor

@crosbymichael Yeah, it is ready :) Just forgot to mention that I pushed my changes

Contributor

LK4D4 commented May 30, 2014

@crosbymichael Yeah, it is ready :) Just forgot to mention that I pushed my changes

@unclejack

This comment has been minimized.

Show comment
Hide comment
@unclejack

unclejack May 30, 2014

Contributor

LGTM

Contributor

unclejack commented May 30, 2014

LGTM

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael May 30, 2014

Contributor

LGTM

Contributor

crosbymichael commented May 30, 2014

LGTM

crosbymichael added a commit that referenced this pull request May 30, 2014

@crosbymichael crosbymichael merged commit 9e58a77 into moby:master May 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@LK4D4 LK4D4 deleted the LK4D4:fix_race_conditions branch May 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment