Repository: stretchr/testify
Pull request #1395 (2023-06-02 21:24:20)

fix: make EventuallyWithT concurrency safe
## Summary

In `assert.EventuallyWithT`, `assert.CollectT` is used in a concurrent context which may lead to races as explained in https://github.com/stretchr/testify/issues/1391.

## Changes

We create a `CollectT` per every condition tick and send its errors over a channel. On the reader side of the channel, we cache the `lastTickErrs` so they can be copied over to t once the timeout is fired. The only access to `lastTickErrs` happens in the reader case and the timeout case which both run in the same goroutine.

## Motivation

We cannot use `EventuallyWithT` family of functions/methods in our codebase due to data races it causes.

<!-- ## Example usage (if applicable) -->

## Related issues
Closes https://github.com/stretchr/testify/issues/1391.

dolmen: So while this effectively serializes calls to CollectT methods (#1418), I'm not convinced this really fixes the core issue.

This implementation doesn't fix the real problem which that we may have multiple instances of the `condition` function running at the same time (overlapping) but we only use a single `CollectT` instance to collect the results, with calls to `Reset` which could alter the result of conditions.

I think we will merge this (with `HasErrors` made private), but work on `EventuallyWithT` must not be considered done.
czeslavo: > So while this effectively serializes calls to CollectT methods (#1418), I'm not convinced this really fixes the core issue.
> 
> This implementation doesn't fix the real problem which that we may have multiple instances of the `condition` function running at the same time (overlapping) but we only use a single `CollectT` instance to collect the results, with calls to `Reset` which could alter the result of conditions.
> 
> I think we will merge this (with `HasErrors` made private), but work on `EventuallyWithT` must not be considered done.

@dolmen Thanks for the comment, you're right! I've taken a look into that again and came up with a solution that doesn't involve a mutex. It also doesn't allow a situation where we end up with an empty `CollectT` on timeout in case of timeout firing up just after `collect.Reset()` call. We create a `CollectT` per every condition tick and send its errors over a channel. On the reader side of the channel, we cache the `lastTickErrs` so they can be copied over to `t` once the timeout is fired. The only access to `lastTickErrs` happens in the reader case and the timeout case which both run in the same goroutine.
dolmen: As the `Reset` and `Copy` methods of `CollectT` are not needed anymore, and they should have been made private in the first place, please replace their implementation with a panic and add a `// Deprecated:` comment.
dolmen: @czeslavo Note for the future, that completely replacing your original implementation on which at least 2 other people worked too (reviewers) with a completely different one, is not a good practice. It would have been better to open a separate PR and close this one.

Anyway, in this case, the original implementation can be forgotten.
dolmen: "address review comments" is never a good commit message. It is completely useless when looking at the Git log or with "git blame". A good commit message must give a summary of what changes, and, if necessary, why. A commit that ONLY says why is never good.

In the present case squashing the commits is the best thing to do.
czeslavo: @dolmen Is there anything else that I could do that's blocking this PR from merging?

Patch:
diff --git a/assert/assertions.go b/assert/assertions.go
index 782c8e5..c276316 100644
--- a/assert/assertions.go
+++ b/assert/assertions.go
@@ -1873,23 +1873,18 @@ func (c *CollectT) Errorf(format string, args ...interface{}) {
 }
 
 // FailNow panics.
-func (c *CollectT) FailNow() {
+func (*CollectT) FailNow() {
 	panic("Assertion failed")
 }
 
-// Reset clears the collected errors.
-func (c *CollectT) Reset() {
-	c.errors = nil
+// Deprecated: That was a method for internal usage that should not have been published. Now just panics.
+func (*CollectT) Reset() {
+	panic("Reset() is deprecated")
 }
 
-// Copy copies the collected errors to the supplied t.
-func (c *CollectT) Copy(t TestingT) {
-	if tt, ok := t.(tHelper); ok {
-		tt.Helper()
-	}
-	for _, err := range c.errors {
-		t.Errorf("%v", err)
-	}
+// Deprecated: That was a method for internal usage that should not have been published. Now just panics.
+func (*CollectT) Copy(TestingT) {
+	panic("Copy() is deprecated")
 }
 
 // EventuallyWithT asserts that given condition will be met in waitFor time,
@@ -1915,8 +1910,8 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
 		h.Helper()
 	}
 
-	collect := new(CollectT)
-	ch := make(chan bool, 1)
+	var lastFinishedTickErrs []error
+	ch := make(chan []error, 1)
 
 	timer := time.NewTimer(waitFor)
 	defer timer.Stop()
@@ -1927,19 +1922,25 @@ func EventuallyWithT(t TestingT, condition func(collect *CollectT), waitFor time
 	for tick := ticker.C; ; {
 		select {
 		case <-timer.C:
-			collect.Copy(t)
+			for _, err := range lastFinishedTickErrs {
+				t.Errorf("%v", err)
+			}
 			return Fail(t, "Condition never satisfied", msgAndArgs...)
 		case <-tick:
 			tick = nil
-			collect.Reset()
 			go func() {
+				collect := new(CollectT)
+				defer func() {
+					ch <- collect.errors
+				}()
 				condition(collect)
-				ch <- len(collect.errors) == 0
 			}()
-		case v := <-ch:
-			if v {
+		case errs := <-ch:
+			if len(errs) == 0 {
 				return true
 			}
+			// Keep the errors from the last ended condition, so that they can be copied to t if timeout is reached.
+			lastFinishedTickErrs = errs
 			tick = ticker.C
 		}
 	}


Test patch:
diff --git a/assert/assertions_test.go b/assert/assertions_test.go
index 0595f21..e1245b4 100644
--- a/assert/assertions_test.go
+++ b/assert/assertions_test.go
@@ -2766,11 +2766,22 @@ func TestEventuallyTrue(t *testing.T) {
 	True(t, Eventually(t, condition, 100*time.Millisecond, 20*time.Millisecond))
 }
 
+// errorsCapturingT is a mock implementation of TestingT that captures errors reported with Errorf.
+type errorsCapturingT struct {
+	errors []error
+}
+
+func (t *errorsCapturingT) Errorf(format string, args ...interface{}) {
+	t.errors = append(t.errors, fmt.Errorf(format, args...))
+}
+
+func (t *errorsCapturingT) Helper() {}
+
 func TestEventuallyWithTFalse(t *testing.T) {
-	mockT := new(CollectT)
+	mockT := new(errorsCapturingT)
 
 	condition := func(collect *CollectT) {
-		True(collect, false)
+		Fail(collect, "condition fixed failure")
 	}
 
 	False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
@@ -2778,7 +2789,7 @@ func TestEventuallyWithTFalse(t *testing.T) {
 }
 
 func TestEventuallyWithTTrue(t *testing.T) {
-	mockT := new(CollectT)
+	mockT := new(errorsCapturingT)
 
 	state := 0
 	condition := func(collect *CollectT) {
@@ -2792,6 +2803,41 @@ func TestEventuallyWithTTrue(t *testing.T) {
 	Len(t, mockT.errors, 0)
 }
 
+func TestEventuallyWithT_ConcurrencySafe(t *testing.T) {
+	mockT := new(errorsCapturingT)
+
+	condition := func(collect *CollectT) {
+		Fail(collect, "condition fixed failure")
+	}
+
+	// To trigger race conditions, we run EventuallyWithT with a nanosecond tick.
+	False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, time.Nanosecond))
+	Len(t, mockT.errors, 2)
+}
+
+func TestEventuallyWithT_ReturnsTheLatestFinishedConditionErrors(t *testing.T) {
+	// We'll use a channel to control whether a condition should sleep or not.
+	mustSleep := make(chan bool, 2)
+	mustSleep <- false
+	mustSleep <- true
+	close(mustSleep)
+
+	condition := func(collect *CollectT) {
+		if <-mustSleep {
+			// Sleep to ensure that the second condition runs longer than timeout.
+			time.Sleep(time.Second)
+			return
+		}
+
+		// The first condition will fail. We expect to get this error as a result.
+		Fail(collect, "condition fixed failure")
+	}
+
+	mockT := new(errorsCapturingT)
+	False(t, EventuallyWithT(mockT, condition, 100*time.Millisecond, 20*time.Millisecond))
+	Len(t, mockT.errors, 2)
+}
+
 func TestNeverFalse(t *testing.T) {
 	condition := func() bool {
 		return false