concurrency errors when printing source.TypedSource

[nicks]

Repro steps:

• Write a custom implementation of source.TypedSource
• Add a Watch on it

Result:
Non-deterministically, controller-runtime blows up with this error:

fatal error: concurrent map iteration and map write

(full stack trace included below)

The Problem

Controller does a fmt.Printf on the Source, which implicitly reads all the internal properties.

controller-runtime/pkg/internal/controller/controller.go

Line 178 in 1ac370e

log := c.LogConstructor(nil).WithValues("source", fmt.Sprintf("%s", watch))

All the built-in Sources implement fmt.Stringer with a concurrency-safe String() string method, so do not have this problem.

Suggestion: the TypedSource interface should also implement fmt.Stringer, so that all implementers have to implement String()

Additional info

Full stack trace:

goroutine 194539 [running]:
reflect.mapiternext(0x18c24c9?)
	/usr/local/go/src/runtime/map.go:1537 +0x13
reflect.(*MapIter).Next(0xc005b6ad38?)
	/usr/local/go/src/reflect/value.go:1989 +0x74
internal/fmtsort.Sort({0x42cf740?, 0xc0202be2c8?, 0xc0267a4380?})
	/usr/local/go/src/internal/fmtsort/sort.go:59 +0x1a7
fmt.(*pp).printValue(0xc012c1b930, {0x42cf740?, 0xc0202be2c8?, 0x5cc9b4207?}, 0x76, 0x3)
	/usr/local/go/src/fmt/print.go:816 +0x988
fmt.(*pp).printValue(0xc012c1b930, {0x4659fe0?, 0xc0202be2a0?, 0xc01f0d7320?}, 0x76, 0x2)
	/usr/local/go/src/fmt/print.go:853 +0x11be
fmt.(*pp).printValue(0xc012c1b930, {0x44e0640?, 0xc0202be2a0?, 0x74cea8f28e70?}, 0x76, 0x1)
	/usr/local/go/src/fmt/print.go:853 +0x11be
fmt.(*pp).printValue(0xc012c1b930, {0x4569ce0?, 0xc0202be2a0?, 0x182a8dd?}, 0x76, 0x0)
	/usr/local/go/src/fmt/print.go:921 +0xae5
fmt.(*pp).badVerb(0xc012c1b930, 0x73)
	/usr/local/go/src/fmt/print.go:394 +0x47f
fmt.(*pp).fmtPointer(0xc012c1b930, {0x4569ce0?, 0xc0202be2a0?, 0x18dcd70?}, 0x73)
	/usr/local/go/src/fmt/print.go:583 +0x414
fmt.(*pp).printValue(0xc012c1b930, {0x4569ce0?, 0xc0202be2a0?, 0x17c6cf0?}, 0x73, 0x2)
	/usr/local/go/src/fmt/print.go:927 +0x1165
fmt.(*pp).printValue(0xc012c1b930, {0x4662ee0?, 0xc02581a9c0?, 0x18dcd70?}, 0x73, 0x1)
	/usr/local/go/src/fmt/print.go:853 +0x11be
fmt.(*pp).printValue(0xc012c1b930, {0x456e1e0?, 0xc02581a9c0?, 0x38?}, 0x73, 0x0)
	/usr/local/go/src/fmt/print.go:921 +0xae5
fmt.(*pp).printArg(0xc012c1b930, {0x456e1e0, 0xc02581a9c0}, 0x73)
	/usr/local/go/src/fmt/print.go:759 +0x4bb
fmt.(*pp).doPrintf(0xc012c1b930, {0x485887b, 0x2}, {0xc005b6beb0, 0x1, 0x1})
	/usr/local/go/src/fmt/print.go:1074 +0x37e
fmt.Sprintf({0x485887b, 0x2}, {0xc00c637eb0, 0x1, 0x1})
	/usr/local/go/src/fmt/print.go:239 +0x53
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2({0x4f37800, 0xc029b08780}, 0xc023ab4790, 0x4f61f00)
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:175 +0x187
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start(0x4f61f00, {0x4f37800, 0xc029b08780})
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:231 +0x186
sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1(0xc02257a840)
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/manager/runnable_group.go:226 +0xc2
created by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile in goroutine 194352
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/manager/runnable_group.go:210 +0x19d

[troy0820]

Extending the interface to ensure that this can be printed, would that be something we would want to enforce? Only because the builtins already satisfy that so there would be no breaking changes but only when we release a new version that we can express for those who have custom sources. Thoughts? @alvaroaleman @sbueringer

[nicks]

Thanks @troy0820 🙏