Refactor State to be 100% thread safe by creack · Pull Request #2798 · moby/moby

creack · 2013-11-21T20:21:54Z

No description provided.

pnasrat · 2013-11-21T20:44:06Z

# github.com/dotcloud/docker
./links_test.go:24: unknown State field 'Running' in struct literal
./links_test.go:69: unknown State field 'Running' in struct literal

pnasrat · 2013-11-21T20:45:52Z

Running with TESTFLAGS="-race" is now much happer.

https://gist.github.com/pnasrat/7589285

pnasrat · 2013-11-21T21:00:20Z

Another build failure is

# github.com/dotcloud/docker/integration
./commands_test.go:292: container.State.Running undefined (type docker.State has no field or method Running)
./commands_test.go:322: l[0].State.Running undefined (type docker.State has no field or method Running)
./commands_test.go:348: container.State.Running undefined (type docker.State has no field or method Running)
./commands_test.go:457: container.State.Running undefined (type docker.State has no field or method Running)
./commands_test.go:537: container.State.Running undefined (type docker.State has no field or method Running)
./commands_test.go:599: container.State.Running undefined (type docker.State has no field or method Running)
./commands_test.go:632: l[0].State.Running undefined (type docker.State has no field or method Running)
./commands_test.go:668: container.State.Running undefined (type docker.State has no field or method Running)
./container_test.go:227: container1.State.Running undefined (type docker.State has no field or method Running)
./container_test.go:233: container1.State.Running undefined (type docker.State has no field or method Running)
./container_test.go:233: too many errors
FAIL    github.com/dotcloud/docker/integration [build failed]

creack · 2013-11-21T21:44:25Z

@pnasrat sorry about that, I open the PR before running the test (building the docker image). Now everything compiles, but sitll have 2 failure.

creack · 2013-11-21T22:37:35Z

All tests now pass. Ping @crosbymichael @vieux

vieux · 2013-11-21T22:53:28Z

Why did you add .UTC() ?

So that way, no matter where you run docker, the date will be consistent.

Don't you think it ca create some issue when we display it in docker ps ?

@vieux If the time zone of the host changes, it might cause problems when the time zone has to be converted from one non-UTC time zone to another non-UTC time zone.

@creack @unclejack True but I don't think this is the pull request to make a change like this. It is out of the scope and could cause issues if users are relying on their local time.

vieux · 2013-11-21T23:05:11Z

I got:

=== RUN TestAttachDetach
--- FAIL: TestAttachDetach (2.32 seconds)
        commands_test.go:510: Impossible to attach to a stopped container, start it first
        commands_test.go:47: First read/write assertion timed out
=== RUN TestAttachDetachTruncatedID
--- FAIL: TestAttachDetachTruncatedID (2.33 seconds)
        commands_test.go:572: Impossible to attach to a stopped container, start it first
        commands_test.go:47: First read/write assertion timed out
=== RUN TestAttachDisconnect
--- FAIL: TestAttachDisconnect (2.06 seconds)
        commands_test.go:47: First read/write assertion timed out
=== RUN TestReloadContainerLinks
--- FAIL: TestReloadContainerLinks (6.42 seconds)
    runtime_test.go:677: Expected 2 container alive, 0 found

Note that TestAttachDetach and TestAttachDetachTruncatedID errors are not the same in master

unclejack · 2013-11-21T23:55:01Z

I'm seeing something similar:

--- FAIL: TestAttachDetach (2.35 seconds)
        commands_test.go:518: Unexpected output. Expected [hello], received [hellohello]
        commands_test.go:47: First read/write assertion timed out
--- FAIL: TestAttachDetachTruncatedID (2.36 seconds)
        commands_test.go:580: Unexpected output. Expected [hello], received [hellohello]
        commands_test.go:47: First read/write assertion timed out

creack · 2013-11-21T23:57:11Z

@unclejack This is an other race. @vieux can you make sure you have the latest commit and try again the first 2 tests?
The 2 other seems non related, I"ll take a look

vieux · 2013-11-21T23:57:13Z

@unclejack those are unrelated hellos are unrelated, they also happen sometimes on master

unclejack · 2013-11-21T23:59:35Z

@vieux @creack Got it, I thought they were worth mentioning since they looked different without this PR and they seem to be failing consistently for me.

pnasrat · 2013-11-22T00:00:56Z

It's possible we've made another race more determistic!

Could someone do a PR to comment all the tests that are known flaky?

Paul

On 21 November 2013 18:59, unclejack [email protected] wrote:

@vieux https://github.com/vieux @creack https://github.com/creack Got
it, I thought they were worth mentioning since they looked different
without this PR and they seem to be failing consistently for me.

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

vieux · 2013-11-22T00:04:57Z

I just succesfully passed the tests. I didn't have the last version.

TestAttachDetach fails randomly, like on master

LGTM

On Thu, Nov 21, 2013 at 4:01 PM, Paul Nasrat [email protected]:

It's possible we've made another race more determistic!

Could someone do a PR to comment all the tests that are known flaky?

Paul

On 21 November 2013 18:59, unclejack [email protected] wrote:

@vieux https://github.com/vieux @creack https://github.com/creack
Got
it, I thought they were worth mentioning since they looked different
without this PR and they seem to be failing consistently for me.

—
Reply to this email directly or view it on GitHub<
https://github.com/dotcloud/docker/pull/2798#issuecomment-29037167>
.

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

Victor VIEUX
http://vvieux.com

crosbymichael · 2013-11-22T00:31:37Z

creack · 2013-11-22T00:35:33Z

ping @crosbymichael, I'll make a separate PR for UTC

crosbymichael · 2013-11-22T00:39:44Z

LGTM

Refactor State to be 100% thread safe

unclejack mentioned this pull request Nov 21, 2013

Lock state before we modify. #2795

Merged

vieux reviewed Nov 21, 2013
View reviewed changes

crosbymichael reviewed Nov 22, 2013
View reviewed changes

Comment thread state.go Outdated

Copy link
Copy Markdown

Contributor

crosbymichael Nov 22, 2013

Choose a reason for hiding this comment

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

Remove UTC

Refactor State to be 100% thread safe

33e7086

crosbymichael added a commit that referenced this pull request Nov 22, 2013

Merge pull request #2798 from dotcloud/fix_state_race

25e443a

Refactor State to be 100% thread safe

crosbymichael merged commit 25e443a into master Nov 22, 2013

crosbymichael deleted the fix_state_race branch November 22, 2013 00:39

Conversation

creack commented Nov 21, 2013

Uh oh!

pnasrat commented Nov 21, 2013

Uh oh!

pnasrat commented Nov 21, 2013

Uh oh!

pnasrat commented Nov 21, 2013

Uh oh!

creack commented Nov 21, 2013

Uh oh!

creack commented Nov 21, 2013

Uh oh!

vieux Nov 21, 2013

Choose a reason for hiding this comment

Uh oh!

creack Nov 21, 2013

Choose a reason for hiding this comment

Uh oh!

vieux Nov 21, 2013

Choose a reason for hiding this comment

Uh oh!

unclejack Nov 21, 2013

Choose a reason for hiding this comment

Uh oh!

crosbymichael Nov 21, 2013

Choose a reason for hiding this comment

Uh oh!

vieux commented Nov 21, 2013

Uh oh!

unclejack commented Nov 21, 2013

Uh oh!

creack commented Nov 21, 2013

Uh oh!

vieux commented Nov 21, 2013

Uh oh!

unclejack commented Nov 21, 2013

Uh oh!

pnasrat commented Nov 22, 2013

Uh oh!

vieux commented Nov 22, 2013

Uh oh!

crosbymichael Nov 22, 2013

Choose a reason for hiding this comment

Uh oh!

creack commented Nov 22, 2013

Uh oh!

crosbymichael commented Nov 22, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants