gomega.Eventually ignores default timeout

[pohly]

In a Kubernetes e2e_node test, we have:

gomega.SetDefaultEventuallyTimeout(5 * time.Minute)
...
gomega.Eventually(ctx, listResources).Should(matchNode, "ResourceSlice from kubelet plugin")

But that call doesn't time out. Eventually the entire suite gets killed by the CI, which doesn't give any information about the reason why it didn't stop.

I think I traced it to:

gomega/internal/async_assertion.go

Lines 330 to 344 in 7cabed6

	func (assertion *AsyncAssertion) afterTimeout() <-chan time.Time {
	if assertion.timeoutInterval >= 0 {
	return time.After(assertion.timeoutInterval)
	}
	if assertion.asyncType == AsyncAssertionTypeConsistently {
	return time.After(assertion.g.DurationBundle.ConsistentlyDuration)
	} else {
	if assertion.ctx == nil {
	return time.After(assertion.g.DurationBundle.EventuallyTimeout)
	} else {
	return nil
	}
	}
	}

assertion.ctx is from Ginkgo and the specific assertion doesn't have a timeout:

(dlv) p assertion.ctx
Sending output to pager...
context.Context(*github.com/onsi/ginkgo/v2/internal.specContext) *{
	Context: context.Context(*context.valueCtx) *{
		Context: context.Context(*context.cancelCtx) ...,
		key: interface {}(string) *(*interface {})(0xc000aa6700),
		val: interface {}(*github.com/onsi/ginkgo/v2/internal.specContext) ...,},
	ProgressReporterManager: *github.com/onsi/ginkgo/v2/internal.ProgressReporterManager {
		lock: *(*sync.Mutex)(0xc000aa44e0),
		progressReporters: map[int]func() string [...],
		prCounter: 1,},
	cancel: context.WithCancelCause.func1 {
		c *context.cancelCtx = *(*context.cancelCtx)(0xc0008141e0)
	},
...

(dlv) p assertion.timeoutInterval
github.com/cenkalti/backoff/v4.Stop (-1)

Therefore the default timeout gets ignored. This seems wrong. gomega.Eventually should stop when the context gets canceled or the gomega.Eventually timeout occurs, whether that was set per call or is the default.

[toshipp]

I encountered the same issue. I found the current behavior was introduced as a fix in e5105cf. It is not intuitive, IMO, but there may be some reason to have done.

[onsi]

hey all - sorry for the delay This was - indeed - an intentional design choice. The thinking was that users choosing to pass in a context would want only the context to govern the timeout for a whole collection of Eventuallys and so layering on an additional non-explicit timeout (ie the default timeout) would be confusing/frustrating.

I'd propose adding a new configuration to Gomega like EnforceDefaultTimeoutsWhenUsingContext() to allow you to opt in to layering the default timeout on top of the context. This would also allow you to turn it on/off on a per spec basis as needed.

The alternative would be for me to detect when the passed in context is a ginkgo context that has no timeout and enforce the default timeout - but that seems too magical (even for me!)

Thoughts?

[pohly]

The thinking was that users choosing to pass in a context would want only the context to govern the timeout for a whole collection of Eventually

That's not how we use Gomega in Kubernetes. We have a global default and then the context typically always is the context straight from Ginkgo, with no extra timeouts added to it. If a gomega.Eventually needs a specific timeout, it gets set through WithTimeout, which is more convenient than the context.WithTimeout + cancel approach.

adding a new configuration to Gomega like EnforceDefaultTimeoutsWhenUsingContext()

That works for me.

[onsi]

So y'all don't use the SpecTimeout(duration) decorators?

[pohly]

No, not in Kubernetes.

[toshipp]

adding a new configuration to Gomega like EnforceDefaultTimeoutsWhenUsingContext()

looks good to me.

So y'all don't use the SpecTimeout(duration) decorators?

No. In my case, It tends to have more than one Eventually. I want to set timeout not to specs but to Eventuallys because I use Eventuallys to wait for something, so the entire spec timeout is the sum of them.

[onsi]

Just added EnforceDefaultTimeoutsWhenUsingContexts() - will cut a release soon.