encoding/json: avoid supurious synctest deadlock detection

Use a sync.OnceValue rather than a sync.WaitGroup to
coordinate access to encoderCache entries.

The OnceValue better expresses the intent of the code
(we want to initialize the cache entry only once).

However, the motivation for this change is to avoid
testing/synctest incorrectly reporting a deadlock
when multiple bubbles call Marshal at the same time.
Goroutines blocked on WaitGroup.Wait are "durably blocked",
causing confusion when a goroutine in one bubble Waits
for a goroutine in a different bubble. Goroutines blocked
on OnceValue are not durably blocked, avoiding the problem.

Fixes #73733
For #67434

Change-Id: I81cddda80af67cf5c280fd4327620bc37e7a6fe6
Reviewed-on: https://go-review.googlesource.com/c/go/+/673335
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/encoding/json/encode.go

@@ -359,25 +359,22 @@ func typeEncoder(t reflect.Type) encoderFunc {
 	}
 
 	// To deal with recursive types, populate the map with an
-	// indirect func before we build it. This type waits on the
-	// real func (f) to be ready and then calls it. This indirect
-	// func is only used for recursive types.
-	var (
-		wg sync.WaitGroup
-		f  encoderFunc
-	)
-	wg.Add(1)
+	// indirect func before we build it. If the type is recursive,
+	// the second lookup for the type will return the indirect func.
+	//
+	// This indirect func is only used for recursive types,
+	// and briefly during racing calls to typeEncoder.
+	indirect := sync.OnceValue(func() encoderFunc {
+		return newTypeEncoder(t, true)
+	})
 	fi, loaded := encoderCache.LoadOrStore(t, encoderFunc(func(e *encodeState, v reflect.Value, opts encOpts) {
-		wg.Wait()
-		f(e, v, opts)
+		indirect()(e, v, opts)
 	}))
 	if loaded {
 		return fi.(encoderFunc)
 	}
 
-	// Compute the real encoder and replace the indirect func with it.
-	f = newTypeEncoder(t, true)
-	wg.Done()
+	f := indirect()
 	encoderCache.Store(t, f)
 	return f
 }

src/encoding/json/encode_test.go

@@ -16,7 +16,9 @@ import (
 	"regexp"
 	"runtime/debug"
 	"strconv"
+	"sync"
 	"testing"
+	"testing/synctest"
 	"time"
 )
 
@@ -1403,3 +1405,21 @@ func TestIssue63379(t *testing.T) {
 		}
 	}
 }
+
+// Issue #73733: encoding/json used a WaitGroup to coordinate access to cache entries.
+// Since WaitGroup.Wait is durably blocking, this caused apparent deadlocks when
+// multiple bubbles called json.Marshal at the same time.
+func TestSynctestMarshal(t *testing.T) {
+	var wg sync.WaitGroup
+	for range 5 {
+		wg.Go(func() {
+			synctest.Test(t, func(t *testing.T) {
+				_, err := Marshal([]string{})
+				if err != nil {
+					t.Errorf("Marshal: %v", err)
+				}
+			})
+		})
+	}
+	wg.Wait()
+}

src/encoding/json/v2_encode_test.go

@@ -16,7 +16,9 @@ import (
 	"regexp"
 	"runtime/debug"
 	"strconv"
+	"sync"
 	"testing"
+	"testing/synctest"
 	"time"
 )
 
@@ -1408,3 +1410,21 @@ func TestIssue63379(t *testing.T) {
 		}
 	}
 }
+
+// Issue #73733: encoding/json used a WaitGroup to coordinate access to cache entries.
+// Since WaitGroup.Wait is durably blocking, this caused apparent deadlocks when
+// multiple bubbles called json.Marshal at the same time.
+func TestSynctestMarshal(t *testing.T) {
+	var wg sync.WaitGroup
+	for range 5 {
+		wg.Go(func() {
+			synctest.Test(t, func(t *testing.T) {
+				_, err := Marshal([]string{})
+				if err != nil {
+					t.Errorf("Marshal: %v", err)
+				}
+			})
+		})
+	}
+	wg.Wait()
+}