Add context-aware YAML unmarshaling

[ccoVeille]

Fixes #187

Replaces #390

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/libyaml/constructor.go:86

• Construct accepts a context.Context, but it is forwarded as-is through the entire decode chain. If a caller passes nil to a *Context entry point, downstream UnmarshalYAML(ctx, ...) implementations can panic when calling ctx.Value, ctx.Done, etc. Consider normalizing nil to context.Background() at the start of Construct so all internal and user callbacks always receive a non-nil context.

func (c *Constructor) Construct(ctx context.Context, n *Node, out reflect.Value) (good bool) {
	c.constructCount++
	if c.aliasDepth > 0 {
		c.aliasCount++
	}

internal/libyaml/node.go:291

• The Node.DecodeContext doc comment has a sentence break/grammar issue (a period followed by a lower-case continuation) and it also implies only structs can implement UnmarshalerContext. Rewording this comment improves clarity for users reading the docs.

// DecodeContext decodes the node and stores its data into the value pointed to by v,
// applying the given context.
//
// The only difference with [Node.Decode] is the ability to receive the provided context to a struct implementing [UnmarshalerContext] interface.
// allowing them to access the context during unmarshalling.

[reuvenharrison]

Thanks @ccoVeille, this is the right redesign, and I am happy to close #390 in favour of it.

#390 stored the context as a field on the Constructor, the pattern the context documentation warns against, and with reason: a constructor shared between goroutines could observe another decode's context. Threading ctx as an explicit parameter costs signature churn but removes that bug class entirely. The stdlib-style names read better too.

One thing worth carrying over from #390: the test pinning the property this buys, that concurrent decodes do not observe each other's contexts. Adapted to this PR's names:

type probeKey struct{}

type ctxProbe struct{ got any }

func (p *ctxProbe) UnmarshalYAML(ctx context.Context, n *yaml.Node) error {
	p.got = ctx.Value(probeKey{})
	return nil
}

func TestLoadContextConcurrent(t *testing.T) {
	var wg sync.WaitGroup
	for i := 0; i < 100; i++ {
		wg.Add(1)
		go func(want int) {
			defer wg.Done()
			ctx := context.WithValue(context.Background(), probeKey{}, want)
			var p ctxProbe
			if err := yaml.LoadContext(ctx, []byte("x"), &p); err != nil {
				t.Error(err)
				return
			}
			if p.got != want {
				t.Errorf("decode observed %v, want %v", p.got, want)
			}
		}(i)
	}
	wg.Wait()
}

Under the field approach this is exactly the test that fails under -race, so it also guards the property against future refactors.

[ccoVeille]

Hi @reuvenharrison

I'm a bit surprised by the code you provided. Yes, it would face race issues, especially when using -race.

But I feel like the -race is usually enough and do not require to spawn an army of Go routines to reveal race conditions.

[reuvenharrison]

The suggested test is more about keeping contexts private to the calling decoder.
My initial implementation let one decode observe a context that another decode had set on the shared constructor - that is what we want to guard against, and it is a wrong-value bug, not only a data race.
The test also provides the concurrency that -race needs: the CI already runs -race, but unless I missed one there is no concurrent test in the suite today, so it currently has nothing to observe. A single small concurrent decode test gives -race something real to check on every run.

[ccoVeille]

I had forgotten to push my changes yesterday.

[ccoVeille]

The suggested test is more about keeping contexts private to the calling decoder. My initial implementation let one decode observe a context that another decode had set on the shared constructor - that is what we want to guard against, and it is a wrong-value bug, not only a data race. The test also provides the concurrency that -race needs: the CI already runs -race, but unless I missed one there is no concurrent test in the suite today, so it currently has nothing to observe. A single small concurrent decode test gives -race something real to check on every run.

I will check in the next weeks, until then I will wait for other people feedback on the PR.