Skip to content

scrape: fix two loop variable scoping bugs in test - #12296

Merged
roidelapluie merged 1 commit into
prometheus:mainfrom
rsc:loopvar
Apr 27, 2023
Merged

scrape: fix two loop variable scoping bugs in test#12296
roidelapluie merged 1 commit into
prometheus:mainfrom
rsc:loopvar

Conversation

@rsc

@rsc rsc commented Apr 26, 2023

Copy link
Copy Markdown
Contributor

Consider code like:

for i := 0; i < numTargets; i++ {
	stopFuncs = append(stopFuncs, func() {
		time.Sleep(i*20*time.Millisecond)
	})
}

Because the loop variable i is shared by all closures, all the stopFuncs sleep for numTargets*20 ms.

If the i were made per-iteration, as we are considering for a future Go release, the stopFuncs would have sleep durations ranging from 0 to (numTargets-1)*20 ms.

Two tests had code like this and were checking that the aggregate sleep was at least numTargets*20 ms
("at least as long as the last target slept"). This is only true today because i == numTarget during all the sleeps.

To keep the code working even if the semantics of this loop change, this PR computes

d := time.Duration((i+1)*20) * time.Millisecond

outside the closure (but inside the loop body), and then each closure has its own d. Now the sleeps range from 20 ms to numTargets*20 ms, keeping the test passing
(and probably behaving closer to the intent of the test author).

The failure being fixed can be reproduced by using the current Go development branch with

GOEXPERIMENT=loopvar go test

Consider code like:

	for i := 0; i < numTargets; i++ {
		stopFuncs = append(stopFuncs, func() {
			time.Sleep(i*20*time.Millisecond)
		})
	}

Because the loop variable i is shared by all closures,
all the stopFuncs sleep for numTargets*20 ms.

If the i were made per-iteration, as we are considering
for a future Go release, the stopFuncs would have sleep
durations ranging from 0 to (numTargets-1)*20 ms.

Two tests had code like this and were checking that the
aggregate sleep was at least numTargets*20 ms
("at least as long as the last target slept"). This is only true
today because i == numTarget during all the sleeps.

To keep the code working even if the semantics of this loop
change, this PR computes

	d := time.Duration((i+1)*20) * time.Millisecond

outside the closure (but inside the loop body), and then each
closure has its own d. Now the sleeps range from 20 ms
to numTargets*20 ms, keeping the test passing
(and probably behaving closer to the intent of the test author).

The failure being fixed can be reproduced by using the current
Go development branch with

	GOEXPERIMENT=loopvar go test

Signed-off-by: Russ Cox <rsc@golang.org>
@roidelapluie
roidelapluie merged commit 24d7e5b into prometheus:main Apr 27, 2023
@roidelapluie

Copy link
Copy Markdown
Member

Thank you for providing such a comprehensive explanation and the solution!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants