Oh, Go. You love your little landmines.

LGTM is what I meant to say.

Merging, since this is clearly a bug, and I think it's what's breaking e2e too.

oh nice find, at first I didn't catch the extra copy.

It's not the extra copy, it's that the pointer is to the loop variable
which keeps getting copied over.

On Tue, Oct 7, 2014 at 9:24 AM, Johan Euphrosine notifications@github.com
wrote:

oh nice find, at first I didn't catch the extra copy.

—
Reply to this email directly or view it on GitHub
#1616 (comment)
.

@brendanburns oh right, I also didn't catch we were passing a pointer to the loop variable.

Good fix. How did this get in in the first place? If a reviewer just missed it, that's fine, it happens--but everyone needs to be aware of this class of problem.

@lavalamp, I think it wasn't caught during review because it was mixed with another "medium" change.
#1534

The last travis build for this change was passing, so I'm not sure if this area is covered:
https://travis-ci.org/GoogleCloudPlatform/kubernetes/builds/37019788

Does go vet or some other linter warn for this? This is EXACTLY what tools
should be warning on. And copying mutexes. And shadowing variables.

On Tue, Oct 7, 2014 at 10:37 AM, Daniel Smith notifications@github.com
wrote:

Good fix. How did this get in in the first place? If a reviewer just
missed it, that's fine, it happens--but everyone needs to be aware of this
class of problem.

Reply to this email directly or view it on GitHub
#1616 (comment)
.

@thockin sounds like good contributions to go vet, but that doesn't really excuse my silly programming mistake :)

But it's not you - I've done this. I've seen it listed as a known landmine
on golang forums.

On Tue, Oct 7, 2014 at 10:48 AM, Johan Euphrosine notifications@github.com
wrote:

@thockin https://github.com/thockin sounds like good contributions to go
vet, but that doesn't really excuse my silly programming mistake :)

Reply to this email directly or view it on GitHub
#1616 (comment)
.

Something else to note is that it is sometime "hard" to detect when you're calling into async code (no excuse here because of the // comment and the podWorkers), that's something to keep in mind for the rest of the codebase and maybe adopt a convention to suffix async function, or make them sync by default and leave it to the caller to wrap them with a goroutine.

Honestly, many of us are Go newbies (myself included), so it's not surprising that this was missed in review.

Or just never use for _, ix { ... } semantics. It's both slower and error
prone, there's not much win there.

--brendan

On Tue, Oct 7, 2014 at 12:41 PM, bgrant0607 notifications@github.com
wrote:

Honestly, many of us are Go newbies (myself included), so it's not
surprising that this was missed in review.

—
Reply to this email directly or view it on GitHub
#1616 (comment)
.

It's legit when looping through a list/map of pointers and not saving
references to the index variable. Other than that, I agree that it's
suboptimal or outright buggy.

On Tue, Oct 7, 2014 at 12:53 PM, Brendan Burns notifications@github.com
wrote:

Or just never use for _, ix { ... } semantics. It's both slower and error
prone, there's not much win there.

--brendan

On Tue, Oct 7, 2014 at 12:41 PM, bgrant0607 notifications@github.com
wrote:

Honestly, many of us are Go newbies (myself included), so it's not
surprising that this was missed in review.

—
Reply to this email directly or view it on GitHub
<
https://github.com/GoogleCloudPlatform/kubernetes/pull/1616#issuecomment-58247499>

.

—
Reply to this email directly or view it on GitHub
#1616 (comment)
.

Yeah, that's true, but it's a hard enough rule to keep in my head (esp.
when a type might be a map under a typedef) that I prefer the cleaner rule
(esp. as a reviewer)

On Tue, Oct 7, 2014 at 12:56 PM, Daniel Smith notifications@github.com
wrote:

It's legit when looping through a list/map of pointers and not saving
references to the index variable. Other than that, I agree that it's
suboptimal or outright buggy.

On Tue, Oct 7, 2014 at 12:53 PM, Brendan Burns notifications@github.com
wrote:

Or just never use for _, ix { ... } semantics. It's both slower and
error
prone, there's not much win there.

--brendan

On Tue, Oct 7, 2014 at 12:41 PM, bgrant0607 notifications@github.com
wrote:

Honestly, many of us are Go newbies (myself included), so it's not
surprising that this was missed in review.

—
Reply to this email directly or view it on GitHub
<

https://github.com/GoogleCloudPlatform/kubernetes/pull/1616#issuecomment-58247499>

.

—
Reply to this email directly or view it on GitHub
<
https://github.com/GoogleCloudPlatform/kubernetes/pull/1616#issuecomment-58249636>

.

—
Reply to this email directly or view it on GitHub
#1616 (comment)
.