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 8 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
86 changes: 54 additions & 32 deletions json/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"sort"
"strconv"
"strings"
"sync"
"sync/atomic"
"time"
"unicode"
Expand All @@ -32,13 +33,29 @@ type codec struct {

type encoder struct {
flags AppendFlags
// ptrDepth tracks the depth of pointer cycles, when it reaches the value
// refDepth tracks the depth of pointer cycles, when it reaches the value
// of startDetectingCyclesAfter, the ptrSeen map is allocated and the
// encoder starts tracking pointers it has seen as an attempt to detect
// whether it has entered a pointer cycle and needs to error before the
// goroutine runs out of stack space.
ptrDepth uint32
ptrSeen map[unsafe.Pointer]struct{}
//
// This relies on encoder being passed as a value,
// and encoder methods calling each other in a traditional stack
// (not using trampoline techniques),
// since refDepth is never decremented.
refDepth uint32
refSeen cycleMap
}

type cycleKey struct {
ptr unsafe.Pointer
len int // 0 for pointers or maps; length for slices or array pointers.
}

type cycleMap map[cycleKey]struct{}

var cycleMapPool = sync.Pool{
New: func() any { return make(cycleMap) },
}

type decoder struct {
Expand All @@ -63,6 +80,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
Copy Markdown
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
Copy Markdown
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 All @@ -85,7 +113,8 @@ func typeid(t reflect.Type) unsafe.Pointer {
}

func constructCachedCodec(t reflect.Type, cache map[unsafe.Pointer]codec) codec {
c := constructCodec(t, map[reflect.Type]*structType{}, t.Kind() == reflect.Ptr)
seen := make(seenMap)
c := constructCodec(t, seen, t.Kind() == reflect.Ptr)

if inlined(t) {
c.encode = constructInlineValueEncodeFunc(c.encode)
Expand All @@ -95,7 +124,14 @@ func constructCachedCodec(t reflect.Type, cache map[unsafe.Pointer]codec) codec
return c
}

func constructCodec(t reflect.Type, seen map[reflect.Type]*structType, canAddr bool) (c codec) {
type seenType struct {
*codec
*structType
}

type seenMap map[reflect.Type]seenType

func constructCodec(t reflect.Type, seen seenMap, canAddr bool) (c codec) {
switch t {
case nullType, nil:
c = codec{encode: encoder.encodeNull, decode: decoder.decodeNull}
Expand All @@ -117,18 +153,6 @@ func constructCodec(t reflect.Type, seen map[reflect.Type]*structType, canAddr b

case rawMessageType:
c = codec{encode: encoder.encodeRawMessage, decode: decoder.decodeRawMessage}

case numberPtrType:
c = constructPointerCodec(numberPtrType, nil)

case durationPtrType:
c = constructPointerCodec(durationPtrType, nil)

case timePtrType:
c = constructPointerCodec(timePtrType, nil)

case rawMessagePtrType:
c = constructPointerCodec(rawMessagePtrType, nil)
}

if c.encode != nil {
Expand Down Expand Up @@ -231,7 +255,7 @@ func constructCodec(t reflect.Type, seen map[reflect.Type]*structType, canAddr b
return
}

func constructStringCodec(t reflect.Type, seen map[reflect.Type]*structType, canAddr bool) codec {
func constructStringCodec(t reflect.Type, seen seenMap, canAddr bool) codec {
c := constructCodec(t, seen, canAddr)
return codec{
encode: constructStringEncodeFunc(c.encode),
Expand All @@ -257,7 +281,7 @@ func constructStringToIntDecodeFunc(t reflect.Type, decode decodeFunc) decodeFun
}
}

func constructArrayCodec(t reflect.Type, seen map[reflect.Type]*structType, canAddr bool) codec {
func constructArrayCodec(t reflect.Type, seen seenMap, canAddr bool) codec {
e := t.Elem()
c := constructCodec(e, seen, canAddr)
s := alignedSize(e)
Expand All @@ -281,7 +305,7 @@ func constructArrayDecodeFunc(size uintptr, t reflect.Type, decode decodeFunc) d
}
}

func constructSliceCodec(t reflect.Type, seen map[reflect.Type]*structType) codec {
func constructSliceCodec(t reflect.Type, seen seenMap) codec {
e := t.Elem()
s := alignedSize(e)

Expand Down Expand Up @@ -348,7 +372,7 @@ func constructSliceDecodeFunc(size uintptr, t reflect.Type, decode decodeFunc) d
}
}

func constructMapCodec(t reflect.Type, seen map[reflect.Type]*structType) codec {
func constructMapCodec(t reflect.Type, seen seenMap) codec {
var sortKeys sortFunc
k := t.Key()
v := t.Elem()
Expand Down Expand Up @@ -466,18 +490,19 @@ func constructMapDecodeFunc(t reflect.Type, decodeKey, decodeValue decodeFunc) d
}
}

func constructStructCodec(t reflect.Type, seen map[reflect.Type]*structType, canAddr bool) codec {
func constructStructCodec(t reflect.Type, seen seenMap, canAddr bool) codec {
st := constructStructType(t, seen, canAddr)
return codec{
encode: constructStructEncodeFunc(st),
decode: constructStructDecodeFunc(st),
}
}

func constructStructType(t reflect.Type, seen map[reflect.Type]*structType, canAddr bool) *structType {
func constructStructType(t reflect.Type, seen seenMap, canAddr bool) *structType {
// Used for preventing infinite recursion on types that have pointers to
// themselves.
st := seen[t]
seenInfo := seen[t]
st := seen[t].structType

if st == nil {
st = &structType{
Expand All @@ -487,7 +512,9 @@ func constructStructType(t reflect.Type, seen map[reflect.Type]*structType, canA
typ: t,
}

seen[t] = st
seenInfo.structType = st
seen[t] = seenInfo

st.fields = appendStructFields(st.fields, t, 0, seen, canAddr)

for i := range st.fields {
Expand Down Expand Up @@ -547,7 +574,7 @@ func constructEmbeddedStructPointerDecodeFunc(t reflect.Type, unexported bool, o
}
}

func appendStructFields(fields []structField, t reflect.Type, offset uintptr, seen map[reflect.Type]*structType, canAddr bool) []structField {
func appendStructFields(fields []structField, t reflect.Type, offset uintptr, seen seenMap, canAddr bool) []structField {
type embeddedField struct {
index int
offset uintptr
Expand Down Expand Up @@ -748,7 +775,7 @@ func encodeKeyFragment(s string, flags AppendFlags) string {
return *(*string)(unsafe.Pointer(&b))
}

func constructPointerCodec(t reflect.Type, seen map[reflect.Type]*structType) codec {
func constructPointerCodec(t reflect.Type, seen seenMap) codec {
e := t.Elem()
c := constructCodec(e, seen, true)
return codec{
Expand Down Expand Up @@ -1092,11 +1119,6 @@ var (
timeType = reflect.TypeOf(time.Time{})
rawMessageType = reflect.TypeOf(RawMessage(nil))

numberPtrType = reflect.PointerTo(numberType)
durationPtrType = reflect.PointerTo(durationType)
timePtrType = reflect.PointerTo(timeType)
rawMessagePtrType = reflect.PointerTo(rawMessageType)

sliceInterfaceType = reflect.TypeOf(([]any)(nil))
sliceStringType = reflect.TypeOf(([]any)(nil))
mapStringInterfaceType = reflect.TypeOf((map[string]any)(nil))
Expand Down
Loading