Skip to content

Lock state before we modify.#2795

Merged
vieux merged 1 commit intomoby:masterfrom
pnasrat:docker-testmultipleattachrestart-race
Nov 21, 2013
Merged

Lock state before we modify.#2795
vieux merged 1 commit intomoby:masterfrom
pnasrat:docker-testmultipleattachrestart-race

Conversation

@pnasrat
Copy link
Copy Markdown
Contributor

@pnasrat pnasrat commented Nov 21, 2013

When we start a container we lock state, we should do the same in stop.

Detected via -race.

See Issue #713

@pnasrat
Lock state before we modify.
d7e2fc8 
When we start a container we lock state, we should do the same in stop.

Detected via -race.
@pnasrat
Copy link
Copy Markdown
Contributor Author

pnasrat commented Nov 21, 2013

We probably want to start using sync.RWMutex http://golang.org/pkg/sync/#RWMutex

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Nov 21, 2013

LGTM

@vieux
Copy link
Copy Markdown
Contributor

vieux commented Nov 21, 2013

LGTM, thanks

vieux pushed a commit that referenced this pull request Nov 21, 2013
Merge pull request #2795 from pnasrat/docker-testmultipleattachrestar…
eaeb969 
…t-race

Lock state before we modify.
@vieux vieux merged commit eaeb969 into moby:master Nov 21, 2013
@pnasrat
Copy link
Copy Markdown
Contributor Author

pnasrat commented Nov 21, 2013

This broke the build

--- FAIL: TestGetContainersExport (5.07 seconds)
    utils_test.go:63: start: Cannot start container 943f421db1840c8cafdd990dcbbbb8880d7b911ad4351f3b152ad8ded60219ab: The container failed to start due to timed out.
=== RUN TestGetContainersChanges

@pnasrat
Copy link
Copy Markdown
Contributor Author

pnasrat commented Nov 21, 2013

@vieux looking at failure. But feel free to revert this.

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Nov 21, 2013

@pnasrat Is this a local build or on the CI ?

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Nov 21, 2013

I ran the tests and there were no issues.

@pnasrat
Copy link
Copy Markdown
Contributor Author

pnasrat commented Nov 21, 2013

build #715 of docker is complete: Failure [failed shell] Build details
are at http://docker-ci.dotcloud.com/builders/docker/builds/715 blamelist:
Paul Nasrat

tianon points out it may just be another flaky test due to another race

Paul

On 21 November 2013 14:40, Michael Crosby [email protected] wrote:

I ran the tests and there were no issues.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2795#issuecomment-29016376
.

@vieux
Copy link
Copy Markdown
Contributor

vieux commented Nov 21, 2013

same here I didn't get any issue

@unclejack
Copy link
Copy Markdown
Contributor

unclejack commented Nov 21, 2013

The following tests are failing for me:

FAIL: TestPostCommit (5.05 seconds)
FAIL: TestBuild (5.56 seconds)
FAIL: TestRunHostname (5.01 seconds)
FAIL: TestRunWorkdirExists (5.06 seconds)
FAIL: TestImagesTree (5.05 seconds)
FAIL: TestDiff (5.06 seconds)
FAIL: TestCommitAutoRun (5.16 seconds)
FAIL: TestRun (5.06 seconds)
FAIL: TestContainerNetwork (5.07 seconds)
FAIL: TestExitCode (5.42 seconds)
FAIL: TestUser (5.06 seconds)
FAIL: TestEntrypointNoCmd (5.06 seconds)
FAIL: TestCopyVolumeContent (5.05 seconds)
FAIL: TestRestartWithVolumes (5.06 seconds)
FAIL: TestVolumesFromWithVolumes (5.17 seconds)
FAIL: TestOnlyLoopbackExistsWhenUsingDisableNetworkOption (5.07 seconds)
FAIL: TestPrivilegedCanMount (5.06 seconds)
FAIL: TestPrivilegedCannotMount (5.05 seconds)
FAIL: TestMultipleVolumesFrom (5.48 seconds)
FAIL: TestRestore (5.83 seconds)
FAIL: TestCreateRmVolumes (5.07 seconds)
FAIL: TestRmi (5.07 seconds)

@pnasrat
Copy link
Copy Markdown
Contributor Author

pnasrat commented Nov 21, 2013

@unclejack out of curiosity how many cores on the machine you're testing on?

@unclejack
Copy link
Copy Markdown
Contributor

unclejack commented Nov 21, 2013

@pnasrat It's a VM with 3 cores.

@crosbymichael crosbymichael mentioned this pull request Nov 21, 2013
Merged
@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Nov 21, 2013

I can reproduce some of the failures that @unclejack is seeing. I opened a PR to revert this change.

It looks like we have found one of the destinations of the races in the code base here.

@pnasrat
Copy link
Copy Markdown
Contributor Author

pnasrat commented Nov 21, 2013

I'll spend some more time on getting this fixed Monday.

@unclejack
Copy link
Copy Markdown
Contributor

unclejack commented Nov 21, 2013

@pnasrat #2798 will fix this.

@pnasrat pnasrat deleted the docker-testmultipleattachrestart-race branch November 21, 2013 20:34
@olljanat olljanat mentioned this pull request Dec 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pnasrat @crosbymichael @vieux @unclejack