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>

diff --git a/src/encoding/json/encode.go b/src/encoding/json/encode.go
index 78d0865b8901abf09f20bf08e031b27ffe58e321..1992e7372ecdd64b48b48000f176ebb4f55643b5 100644 (file)
--- a/src/encoding/json/encode.go
+++ b/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
 }

diff --git a/src/encoding/json/encode_test.go b/src/encoding/json/encode_test.go
index bc31f9d48a4e8064dd93aec0ec035dce66cde8a3..87074eabd4639934fca4ea4f58154f72892cc286 100644 (file)
--- a/src/encoding/json/encode_test.go
+++ b/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()
+}

diff --git a/src/encoding/json/v2_encode_test.go b/src/encoding/json/v2_encode_test.go
index 16e8d0121804e75d20db573b34a3e6080848f7bd..11c521864916e04db6bb036102829720ab1e990a 100644 (file)
--- a/src/encoding/json/v2_encode_test.go
+++ b/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()
+}