New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WaitFor returns immediately when done is closed #72364
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -456,11 +456,42 @@ func TestWaitFor(t *testing.T) { | |
} | ||
} | ||
|
||
// TestWaitForWithEarlyClosingWaitFunc tests WaitFor when the WaitFunc closes its channel. The WaitFor should | ||
// always return ErrWaitTimeout. | ||
func TestWaitForWithEarlyClosingWaitFunc(t *testing.T) { | ||
stopCh := make(chan struct{}) | ||
defer close(stopCh) | ||
|
||
start := time.Now() | ||
err := WaitFor(func(done <-chan struct{}) <-chan struct{} { | ||
c := make(chan struct{}) | ||
close(c) | ||
return c | ||
}, func() (bool, error) { | ||
return false, nil | ||
}, stopCh) | ||
duration := time.Now().Sub(start) | ||
|
||
// The WaitFor should return immediately, so the duration is close to 0s. | ||
if duration >= ForeverTestTimeout/2 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why dividing it by 2 instead of using ForeverTestTimeout directly? Is the purpose to make the test less false negative (i.e., less likely to miss a bug)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
t.Errorf("expected short timeout duration") | ||
} | ||
if err != ErrWaitTimeout { | ||
t.Errorf("expected ErrWaitTimeout from WaitFunc") | ||
} | ||
} | ||
|
||
// TestWaitForWithClosedChannel tests WaitFor when it receives a closed channel. The WaitFor should | ||
// always return ErrWaitTimeout. | ||
func TestWaitForWithClosedChannel(t *testing.T) { | ||
stopCh := make(chan struct{}) | ||
close(stopCh) | ||
c := make(chan struct{}) | ||
defer close(c) | ||
start := time.Now() | ||
err := WaitFor(poller(ForeverTestTimeout, ForeverTestTimeout), func() (bool, error) { | ||
err := WaitFor(func(done <-chan struct{}) <-chan struct{} { | ||
return c | ||
}, func() (bool, error) { | ||
return false, nil | ||
}, stopCh) | ||
duration := time.Now().Sub(start) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing this to 1 second means the time.Sleep of 1 second is now too short to reliably catch sync problems. The test should be able to wait significantly longer than the resync period to ensure progress is made when expected. Having to lengthen the resync period this much means having to extend the test wait time significantly, which makes the test take much longer to run than desired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the original 100x difference was to ensure the gc sync was run ~100 times to catch any flakiness.
How about setting it 200ms here, and let the test run 10s (by setting the sleep time to 10s), then the sync got tested 50 times?
Also note that the old test wasn't as reliable as it intended to be. Although the sync period was set to 10ms, because the old
WaitFor
function didn't handle the closeddone
channel, the WaitForCacheSync returned 100ms later, after the first poll period. So the old test only run the sync behavior 10 times in the 1s test time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was actually to make sure the test waited waaaaay longer than the resync period to ensure there was time to complete at least 2 iterations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pseudo-code of
GarbageCollector.Sync()
:1 second of period makes controller.WaitForCacheSync() tries ~10 times to check if the cache is synced.
200ms of period makes ~2 executions of
controller.WaitForCacheSync()
in every wait.PollImmediateUntil() loop. Finallycontroller.WaitForCacheSync()
also executes about 10 times. (Due to the behavior of time.Ticker, it may drop some ticks for slow receivers. So the 10 is the upper bound)200ms is better because it tests both of
wait.PollImmediateUntil()
andcontroller.WaitForCacheSync()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's probably worth documenting in a comment in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@kdada could you add the comment as liggitt suggested? The suedo-code is great, it makes the reasoning much more clear. I added a little more details to it.
The final goal is to make sure that the outermost wait.Until loop runs at least 2 times during the 1s sleep, which ensures that the changes made to the fakeDiscoveryClient are picked up by the Sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caesarxuchao Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm