Hide the mutex lock in formatter.ContainerStats by boaz0 · Pull Request #26819 · moby/moby

boaz0 · 2016-09-22T13:57:44Z

- What I did
Created getters and setters to the container statistics and hide the mutex lock.

- How I did it

Hide the mutex field in formatter.ContainerStats.
Expose setters and getters that return or set the statistics using that mutex.

- How to verify it

Run the integration and unit tests
Run docker stats and docker stats --format "{{.PIDs}} -- {{.Container}}" and see that the formats are different.

- Description for the changelog
This patch supposes to fix the issue mentioned in #24987

thaJeztah · 2016-09-22T14:00:55Z

ping @runcom @cpuguy83 PTAL

boaz0 · 2016-09-22T14:19:08Z

I hate the idea of copying the containers statistics (it won't scale nicely).
I am working on a better way to avoid this part.

boaz0 · 2016-09-25T12:50:20Z

@thaJeztah @runcom @cpuguy83 @vdemeester @LK4D4
Can you please review this change?

Any feedback will be appreciated! 😺
Thanks a lot!

LK4D4 · 2016-09-26T14:54:40Z

@ripcurld00d Doesn't this mean that stats could get CPU and memory from different time frames? I sorta suspicious about granular locking for setting parts of stats, it adds a lot of code and has little sense IMO, because stats structure supposed to be "atomic" for ps(also repeated lock/unlock might have performance issues). Maybe I'm missing something, though.

boaz0 · 2016-09-26T14:59:09Z

@LK4D4, no you got it right. I do believe I can make this more atomic (read the CPU, memory and etc in one time frame), though.

Anything else that I should keep in mind?

LK4D4 · 2016-09-26T15:01:02Z

@ripcurld00d Otherwise, patch looks good to me.

boaz0 · 2016-09-27T10:18:32Z

@LK4D4 I updated the commit, feel free to give new feedback.
/cc @runcom @cpuguy83
😀

LK4D4 · 2016-09-27T14:21:22Z

This name isn't good IMO. At first, it doesn't reflect what this structure does and then "clear containers" is intel containers technology :)

Yes, I am bad at giving names 😊.
Is it a bad idea to move this to api/types and call it ContainerStats?

I dunno :) ping @vdemeester @thaJeztah

What's the difference with ContainerStats? (Just so I can try thinking of a better name)

@thaJeztah ContainerStats is used for collecting the container statistics while ClearContainerStats (now ContainerStatsRecord) is used to display the statistics on the screen.

Because of that ContainerStats has both mutex and an error field.

LK4D4 · 2016-09-27T14:23:46Z

Can't you make statistics type public and use it here as argument?

Yes, it will definitely make it more readable. 👍

boaz0 · 2016-09-29T11:28:42Z

Odd, windowRS1 failed:

10:59:05 FAIL: docker_cli_run_test.go:1855: DockerSuite.TestRunInteractiveWithRestartPolicy
               docker_cli_run_test.go:1868:
               Failures:
               ExitCode was 0 expected 11
               Expected command to finish, but it hit the timeout

10:18:24 FAIL: docker_cli_events_test.go:81: DockerSuite.TestEventsLimit
               docker_cli_events_test.go:102:
               value *errors.errorString = &errors.errorString{s:"exit status 125: d:\\CI\\CI-7bc23d0\\binary\\docker.exe: Error response from daemon: container f885f96b146b7d711d78d27a0f100c72a5d18fc72a340359c376e1c77f5c1424 encountered an error during Start failed in Win32: This operation returned because the timeout period expired. (0x5b4).\n"} ("exit status 125: d:\\CI\\CI-7bc23d0\\binary\\docker.exe: Error response from daemon: container f885f96b146b7d711d78d27a0f100c72a5d18fc72a340359c376e1c77f5c1424 encountered an error during Start failed in Win32: This operation returned because the timeout period expired. (0x5b4).\n")

None of them are related to docker stats.
Is my commit the only one who failed on those tests? 😟

cpuguy83

I can sort of follow the code, but it's a bit confusing.

I wonder if we need so many different structs for the same information.
Instead we can put methods on one, like IsWindows() which can lock and check the windows field, and Name() and read the name field.

cpuguy83 · 2016-09-29T12:03:04Z

I think this comment is not right.
Statistics is where this comment belongs.

Still not quite sure what this does. Maybe it's just for formatting?
Maybe ContainerStatsFormatter?

👍 sounds right

cpuguy83 · 2016-09-29T12:03:34Z

I think maybe a better name would be StatsEntry

cpuguy83 · 2016-09-29T12:06:10Z

All of these with basically the same fields and name is pretty confusing.

cpuguy83 · 2016-09-29T12:07:07Z

Not really "getting" anything here, just copying into a new struct.

Yes, that's right. Nevertheless, you eventually get a ContainerStatsRecord entity.
You want me to rename this anyway?

Doesn't matter.

boaz0 · 2016-09-29T14:35:20Z

@cpuguy83 thanks for your feedback.

boaz0 · 2016-10-09T20:14:46Z

@cpuguy83 and @LK4D4 PTAL thanks. 😺

/cc @thaJeztah

LK4D4 · 2016-10-10T16:55:16Z

Let's use values instead of pointers everywhere for consistency.

👍 no problem

LK4D4 · 2016-10-10T17:02:16Z

I don't like global variable usage here. It's definitely shouldn't be global.

Ummm... the alternative is to put this variable inside the StatsEntity struct.
What do you think?

Yeah, I think it would be better. I saw that we use global var in stat_helper too. It's also so wrong :(

👍
I don't mind fixing this as well.
The question is whether you prefer doing that in this commit or in another one.

Let's do it in another PR.

no problem ☕

The formatter.ContainerStats struct exposes its Mutex. This is a bad design and should be fixed. To fix that, I separated the statistics attributes from ContainerStats to StatsEntry and hid the mutex. Notice that the mutex protects both the `err` field and the statistics attributes. Then, implemented SetStatistics, SetError, GetStatistics and GetError to avoid races. Moreover, to make this less granular, I decided to replace the read-write mutex with the regular mutex and to pass a StatsEntry slice to formatter.ContainerStatsWrite Signed-off-by: Boaz Shuster <[email protected]>

boaz0 · 2016-10-10T20:14:18Z

@LK4D4 re-review 🎱 🍶 plz?

LK4D4 · 2016-10-10T20:43:13Z

ping @cpuguy83

tonistiigi · 2016-10-13T17:13:18Z

LGTM

thaJeztah

LGTM

Hide the mutex lock in formatter.ContainerStats

GordonTheTurtle added status/0-triage dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Sep 22, 2016

thaJeztah added status/2-code-review and removed status/0-triage labels Sep 22, 2016

thaJeztah added this to the 1.13.0 milestone Sep 22, 2016

boaz0 commented Sep 22, 2016

View reviewed changes

boaz0 changed the title ~~Remove the mutex lock from formatter.ContainerStats~~ Hide the mutex lock in formatter.ContainerStats Sep 25, 2016

LK4D4 suggested changes Sep 27, 2016

View reviewed changes

cpuguy83 requested changes Sep 29, 2016

View reviewed changes

LK4D4 suggested changes Oct 10, 2016

View reviewed changes

LK4D4 approved these changes Oct 10, 2016

View reviewed changes

thaJeztah approved these changes Oct 14, 2016

View reviewed changes

thaJeztah merged commit ba2c13f into moby:master Oct 14, 2016

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#26819 from ripcurld00d/fix_stats_mutex

bfe16dc

Hide the mutex lock in formatter.ContainerStats

thaJeztah mentioned this pull request Sep 11, 2025

cli/command/container: prevent panic during stats on empty event Actor.ID docker/cli#6451

Merged

thaJeztah mentioned this pull request Sep 24, 2025

[28.x backport] cli/command/container: prevent panic during stats on empty event Actor.ID docker/cli#6471

Merged

Conversation

boaz0 commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Sep 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Sep 25, 2016

Uh oh!

LK4D4 commented Sep 26, 2016

Uh oh!

boaz0 commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LK4D4 commented Sep 26, 2016

Uh oh!

boaz0 commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Sep 29, 2016

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boaz0 commented Oct 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Sep 22, 2016 •

edited

Loading

boaz0 commented Sep 26, 2016 •

edited

Loading

boaz0 commented Sep 27, 2016 •

edited

Loading

boaz0 Sep 29, 2016 •

edited

Loading

boaz0 commented Sep 29, 2016 •

edited

Loading