Skip to content

json: fix cycle detection #153

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.

Sign up for GitHub

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

Open
extemporalgenome wants to merge 10 commits into
base: master
Choose a base branch
from
Open

json: fix cycle detection #153

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e960845
json: consistent use of encodeNull (should inline well)
extemporalgenome Jul 29, 2025
dabc507
json: internal uses of Append now use new encoder.appendAny
extemporalgenome Jul 28, 2025
f786a42
json: refactor ref-cycle handling
extemporalgenome Jul 28, 2025
d8717df
json: support cycle detection involving maps
extemporalgenome Jul 28, 2025
722d2e3
json: detect slice-based value cycles
extemporalgenome Nov 25, 2025
b34b8b9
json: remove unnecessary bespoke pointer types
extemporalgenome Nov 27, 2025
41dfddc
json: seen data structure used in codec building supports non-structs
extemporalgenome Nov 27, 2025
40c79bc
json: update cycle checking aspects from stdlib golang_encode_test.go
extemporalgenome Jul 28, 2025
396975c
json: codec is passed as a pointer
extemporalgenome Nov 30, 2025
9aaaa4b
json: pass remaining cycle/unsupported-value tests
extemporalgenome Nov 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions json/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,17 @@ type (
// lookup time for simple types like bool, int, etc..
var cache atomic.Pointer[map[unsafe.Pointer]codec]

func cachedCodec(t reflect.Type) codec {
cache := cacheLoad()

c, found := cache[typeid(t)]
Copy link
Contributor

@kevinburkesegment kevinburkesegment Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread safety for this access, or is "single thread assumed" documented somewhere higher up?

Can you fix the typo on line 62 - "Marshal"

Copy link
Contributor Author

@extemporalgenome extemporalgenome Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This concurrency behavior was already in place within this json package, and the decision is maintained within this PR despite some minor refactoring around the concept.

Essentially the "global cache" is stored in an atomic.Pointer wrapping a map. To access the cache, the map is atomically loaded but then read normally. To update the cache, we first create a shallow copy of the map (cached values are never modified), then set our new key(s), then atomically store the new map in place of the old one.

Thus reads concurrent with writes are okay: the reader already has what it needs from whichever map it had loaded.
Concurrent writes are also okay: if both goroutines needed to handle different types which hadn't already been in the map, both will build and use codecs for the immediate operation they're performing, and then the last writer will "win" the race, keeping its particular codec in the cache for next time. The type that lost the race will get regenerated in a future call and eventually end up durably in the cache.

The code comments aptly refer to this as an eventually consistent cache, because over time, with enough repeat calls, there will be fewer and fewer benign races each time (any type that wins a race never needs to participate in the race again), and eventually every needed type will end up in the cache.

if !found {
c = constructCachedCodec(t, cache)
}

return c
}

func cacheLoad() map[unsafe.Pointer]codec {
p := cache.Load()
if p == nil {
Expand Down
46 changes: 32 additions & 14 deletions json/encode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"math"
"reflect"
"runtime"
"sort"
"strconv"
"sync"
Expand All @@ -17,6 +18,23 @@ import (

const hex = "0123456789abcdef"

func (e encoder) appendAny(b []byte, x any) ([]byte, error) {
if x == nil {
// Special case for nil values because it makes the rest of the code
// simpler to assume that it won't be seeing nil pointers.
return e.encodeNull(b, nil)
}

t := reflect.TypeOf(x)
p := (*iface)(unsafe.Pointer(&x)).ptr
c := cachedCodec(t)

b, err := c.encode(e, b, p)
runtime.KeepAlive(x)

return b, err
}

func (e encoder) encodeNull(b []byte, p unsafe.Pointer) ([]byte, error) {
return append(b, "null"...), nil
}
Expand Down Expand Up @@ -241,7 +259,7 @@ func (e encoder) encodeToString(b []byte, p unsafe.Pointer, encode encodeFunc) (
func (e encoder) encodeBytes(b []byte, p unsafe.Pointer) ([]byte, error) {
v := *(*[]byte)(p)
if v == nil {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

n := base64.StdEncoding.EncodedLen(len(v)) + 2
Expand Down Expand Up @@ -299,7 +317,7 @@ func (e encoder) encodeSlice(b []byte, p unsafe.Pointer, size uintptr, t reflect
s := (*slice)(p)

if s.data == nil && s.len == 0 && s.cap == 0 {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

return e.encodeArray(b, s.data, s.len, size, t, encode)
Expand All @@ -308,7 +326,7 @@ func (e encoder) encodeSlice(b []byte, p unsafe.Pointer, size uintptr, t reflect
func (e encoder) encodeMap(b []byte, p unsafe.Pointer, t reflect.Type, encodeKey, encodeValue encodeFunc, sortKeys sortFunc) ([]byte, error) {
m := reflect.NewAt(t, p).Elem()
if m.IsNil() {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

keys := m.MapKeys()
Expand Down Expand Up @@ -363,7 +381,7 @@ var mapslicePool = sync.Pool{
func (e encoder) encodeMapStringInterface(b []byte, p unsafe.Pointer) ([]byte, error) {
m := *(*map[string]any)(p)
if m == nil {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

if (e.flags & SortMapKeys) == 0 {
Expand All @@ -383,7 +401,7 @@ func (e encoder) encodeMapStringInterface(b []byte, p unsafe.Pointer) ([]byte, e
b, _ = e.encodeString(b, unsafe.Pointer(&k))
b = append(b, ':')

b, err = Append(b, v, e.flags)
b, err = e.appendAny(b, v)
if err != nil {
return b, err
}
Expand Down Expand Up @@ -417,7 +435,7 @@ func (e encoder) encodeMapStringInterface(b []byte, p unsafe.Pointer) ([]byte, e
b, _ = e.encodeString(b, unsafe.Pointer(&elem.key))
b = append(b, ':')

b, err = Append(b, elem.val, e.flags)
b, err = e.appendAny(b, elem.val)
if err != nil {
break
}
Expand All @@ -441,7 +459,7 @@ func (e encoder) encodeMapStringInterface(b []byte, p unsafe.Pointer) ([]byte, e
func (e encoder) encodeMapStringRawMessage(b []byte, p unsafe.Pointer) ([]byte, error) {
m := *(*map[string]RawMessage)(p)
if m == nil {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

if (e.flags & SortMapKeys) == 0 {
Expand Down Expand Up @@ -520,7 +538,7 @@ func (e encoder) encodeMapStringRawMessage(b []byte, p unsafe.Pointer) ([]byte,
func (e encoder) encodeMapStringString(b []byte, p unsafe.Pointer) ([]byte, error) {
m := *(*map[string]string)(p)
if m == nil {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

if (e.flags & SortMapKeys) == 0 {
Expand Down Expand Up @@ -586,7 +604,7 @@ func (e encoder) encodeMapStringString(b []byte, p unsafe.Pointer) ([]byte, erro
func (e encoder) encodeMapStringStringSlice(b []byte, p unsafe.Pointer) ([]byte, error) {
m := *(*map[string][]string)(p)
if m == nil {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

stringSize := unsafe.Sizeof("")
Expand Down Expand Up @@ -667,7 +685,7 @@ func (e encoder) encodeMapStringStringSlice(b []byte, p unsafe.Pointer) ([]byte,
func (e encoder) encodeMapStringBool(b []byte, p unsafe.Pointer) ([]byte, error) {
m := *(*map[string]bool)(p)
if m == nil {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

if (e.flags & SortMapKeys) == 0 {
Expand Down Expand Up @@ -813,11 +831,11 @@ func (e encoder) encodePointer(b []byte, p unsafe.Pointer, t reflect.Type, encod
}

func (e encoder) encodeInterface(b []byte, p unsafe.Pointer) ([]byte, error) {
return Append(b, *(*any)(p), e.flags)
return e.appendAny(b, *(*any)(p))
}

func (e encoder) encodeMaybeEmptyInterface(b []byte, p unsafe.Pointer, t reflect.Type) ([]byte, error) {
return Append(b, reflect.NewAt(t, p).Elem().Interface(), e.flags)
return e.appendAny(b, reflect.NewAt(t, p).Elem().Interface())
}

func (e encoder) encodeUnsupportedTypeError(b []byte, p unsafe.Pointer, t reflect.Type) ([]byte, error) {
Expand All @@ -828,7 +846,7 @@ func (e encoder) encodeRawMessage(b []byte, p unsafe.Pointer) ([]byte, error) {
v := *(*RawMessage)(p)

if v == nil {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}

var s []byte
Expand Down Expand Up @@ -862,7 +880,7 @@ func (e encoder) encodeJSONMarshaler(b []byte, p unsafe.Pointer, t reflect.Type,
switch v.Kind() {
case reflect.Ptr, reflect.Interface:
if v.IsNil() {
return append(b, "null"...), nil
return e.encodeNull(b, nil)
}
}

Expand Down
30 changes: 4 additions & 26 deletions json/json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"io"
"math/bits"
"reflect"
"runtime"
"sync"
"unsafe"
)
Expand Down Expand Up @@ -194,25 +193,9 @@ func (k Kind) Class() Kind { return Kind(1 << uint(bits.Len(uint(k))-1)) }
// Append acts like Marshal but appends the json representation to b instead of
// always reallocating a new slice.
func Append(b []byte, x any, flags AppendFlags) ([]byte, error) {
if x == nil {
// Special case for nil values because it makes the rest of the code
// simpler to assume that it won't be seeing nil pointers.
return append(b, "null"...), nil
}

t := reflect.TypeOf(x)
p := (*iface)(unsafe.Pointer(&x)).ptr

cache := cacheLoad()
c, found := cache[typeid(t)]

if !found {
c = constructCachedCodec(t, cache)
}
e := encoder{flags: flags}

b, err := c.encode(encoder{flags: flags}, b, p)
runtime.KeepAlive(x)
return b, err
return e.appendAny(b, x)
}

// Escape is a convenience helper to construct an escaped JSON string from s.
Expand Down Expand Up @@ -330,14 +313,9 @@ func Parse(b []byte, x any, flags ParseFlags) ([]byte, error) {
}
return r, &InvalidUnmarshalError{Type: t}
}
t = t.Elem()

cache := cacheLoad()
c, found := cache[typeid(t)]

if !found {
c = constructCachedCodec(t, cache)
}
t = t.Elem()
c := cachedCodec(t)

r, err := c.decode(d, b, p)
return skipSpaces(r), err
Expand Down