Skip to content
Open
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,21 @@ enrichedCtx, _ := evaluator.AddDataToContext(context.Background(), runtimeData)
result, _ := evaluator.Eval(enrichedCtx)
```

#### Concurrency and thread-safety

`ContextProvider.AddDataToContext` is safe for concurrent use. Each call returns a derived context independent of its parent: data merged into the derived context never affects the parent or any sibling derived chain.

Existing data from the parent context is deep-copied at every nesting level when forming the derived context, so concurrent enrichments from goroutines that share a parent context — even one already populated with nested maps — do not race on shared inner storage.

The typical pattern is one derived-context chain per request, with the returned context threaded forward for any subsequent enrichment:

```go
ctx, _ := provider.AddDataToContext(parent, requestData)
ctx, _ = provider.AddDataToContext(ctx, moreData) // build on the returned ctx
```

For the details of which value kinds are deep-copied versus stored by reference, see the [`AddDataToContext` godoc](https://pkg.go.dev/github.com/robbyt/go-polyscript/platform/data#ContextProvider.AddDataToContext).

### Combining Static and Dynamic Runtime Data

Use the following pattern for fixed configuration values and per-request data. Initial loading, parsing, and instantiating the script is relatively slow, so the example below shows how to set up the script once with static data and then reuse it many times with dynamic runtime data.
Expand Down
58 changes: 56 additions & 2 deletions platform/data/contextProvider.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"maps"
"net/http"

"github.com/robbyt/go-polyscript/internal/helpers"
Expand Down Expand Up @@ -50,6 +49,32 @@ func (p *ContextProvider) GetData(ctx context.Context) (map[string]any, error) {
// and later values override earlier ones for duplicate keys.
//
// See README.md for detailed usage examples.
//
// # Concurrency
//
// AddDataToContext is safe for concurrent use. Each call returns a
// derived context independent of its parent: data merged into the
// derived context does not affect the parent or any sibling derived
// chain. Existing data from the parent context is deep-copied at every
// nesting level when forming the derived context, so concurrent
// enrichments from goroutines that share a parent never race on shared
// inner map storage.
//
// Reference-typed values stored in the map (slices, pointers, channels,
// function values, and maps OTHER than map[string]any) remain shared
// with the source — ContextProvider never mutates them in place, but
// callers should avoid mutating them after handing them to
// AddDataToContext. The deep-copy guarantee applies specifically to
// map[string]any values, which processValue and deepCopyMap recurse
// into; other map types (e.g. map[string]string, map[any]any) are
// stored as opaque values. Value types (int, string, struct, etc.) are
// copied as usual when stored in an `any` slot. *http.Request and
// http.Request inputs are converted to map[string]any via
// helpers.RequestToMap on the way in, so the underlying request is not
// stored.
Comment thread
robbyt marked this conversation as resolved.
//
// The typical pattern is one derived-context chain per request, with
// the returned context threaded forward for any subsequent enrichment.
func (p *ContextProvider) AddDataToContext(
ctx context.Context,
data ...map[string]any,
Expand All @@ -63,7 +88,10 @@ func (p *ContextProvider) AddDataToContext(

if existingData := ctx.Value(p.contextKey); existingData != nil {
if existingMap, ok := existingData.(map[string]any); ok {
maps.Copy(toStore, existingMap)
// Deep-copy nested maps so subsequent in-place merge into
// toStore cannot race with goroutines reading or merging the
// same parent context. See the # Concurrency note above.
toStore = deepCopyMap(existingMap)
}
}

Expand Down Expand Up @@ -147,3 +175,29 @@ func (p *ContextProvider) mergeIntoMap(
// Non-map values simply replace existing values
target[key] = value
}

// deepCopyMap returns a recursive copy of m. Nested map[string]any values
// are allocated fresh at every level so the returned map shares no mutable
// map storage with m.
//
// Always returns a non-nil map, even when m is nil or empty; this lets
// AddDataToContext use the result directly without a separate guard
// against a nil parent-context value.
//
// Non-map values are stored by assignment to an `any` slot. Value types
// (int, string, struct, etc.) are copied as usual; reference types
// (slices, pointers, channels, function values) remain shared with the
// source. ContextProvider never mutates stored values in place; callers
// should avoid mutating reference-typed values after handing them to
// AddDataToContext.
func deepCopyMap(m map[string]any) map[string]any {
out := make(map[string]any, len(m))
for k, v := range m {
if nested, ok := v.(map[string]any); ok {
out[k] = deepCopyMap(nested)
} else {
out[k] = v
}
}
return out
}
106 changes: 106 additions & 0 deletions platform/data/contextProvider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

import (
"context"
"fmt"
"net/http"
"sync"
"testing"

"github.com/robbyt/go-polyscript/platform/constants"
Expand Down Expand Up @@ -583,3 +585,107 @@
assert.Contains(t, err.Error(), "empty keys are not allowed")
})
}

// TestContextProvider_AddDataToContext_TypedNilParent guards against a
// regression caught in Copilot review of #93: if a typed-nil map[string]any
// is stored in the context value (e.g. `context.WithValue(parent, key,
// (map[string]any)(nil))`), the parent map ranges as empty, deepCopyMap
// must still produce a usable non-nil destination, and the subsequent
// merge must not panic with "assignment to entry in nil map".
func TestContextProvider_AddDataToContext_TypedNilParent(t *testing.T) {
t.Parallel()
p := NewContextProvider(constants.EvalData)

var typedNil map[string]any
parent := context.WithValue(t.Context(), constants.EvalData, typedNil)

// This call would have panicked under the early-return-nil version
// of deepCopyMap (toStore = nil → mergeIntoMap → assignment to entry
// in nil map).
next, err := p.AddDataToContext(parent, map[string]any{"k": "v"})
require.NoError(t, err)

got, err := p.GetData(next)
require.NoError(t, err)
assert.Equal(t, "v", got["k"])
}

// TestContextProvider_AddDataToContext_Concurrent is the regression test
// for the deep-copy fix in #93. Goroutines share an already-enriched
// parent context whose value contains a nested map. Each goroutine
// enriches the SAME nested key with its own data and reads it back.
// Before the deep-copy fix, the recursive merge mutated the nested map
// in-place — concurrent goroutines raced on shared inner storage and
// -race would fire. The fix deep-copies nested maps when deriving from
// the parent so each derived chain owns its own nested map storage.
//
// 32 goroutines × 100 iterations = 3200 enrichment + read-back checks
// against a shared parent. Passes cleanly under -race.
func TestContextProvider_AddDataToContext_Concurrent(t *testing.T) {

Check failure on line 624 in platform/data/contextProvider_test.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 24 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=robbyt_go-polyscript&issues=AZ4kA5ZhWBPibYOuM1_b&open=AZ4kA5ZhWBPibYOuM1_b&pullRequest=128
t.Parallel()

p := NewContextProvider(constants.EvalData)

// Pre-enrich a shared root with a nested map. This is the exact
// shape that was unsafe under the old shallow-copy implementation.
parent, err := p.AddDataToContext(t.Context(), map[string]any{
"shared": map[string]any{"baseline": "from-parent"},
})
require.NoError(t, err)

const goroutines = 32
const iterations = 100

var wg sync.WaitGroup
wg.Add(goroutines)
for id := range goroutines {
go func() {
defer wg.Done()
Comment thread
robbyt marked this conversation as resolved.

// All goroutines start from the SAME enriched parent.
ctx := parent
for j := range iterations {
key := fmt.Sprintf("g%d-iter%d", id, j)
Comment thread
robbyt marked this conversation as resolved.

// Enrich the nested "shared" map with our own key. Before
// the fix this was the racing path: mergeIntoMap recursed
// into parent.shared and mutated it concurrently.
next, err := p.AddDataToContext(ctx, map[string]any{
"shared": map[string]any{key: j},
})
if !assert.NoErrorf(t, err, "goroutine %d iter %d", id, j) {
return
}

// Read-back invariant: our derived chain must contain
// our key under "shared", plus the baseline from parent.
got, err := p.GetData(next)
if !assert.NoErrorf(t, err, "goroutine %d iter %d GetData", id, j) {
return
}
shared, ok := got["shared"].(map[string]any)
if !assert.Truef(t, ok, "goroutine %d iter %d: shared not a map", id, j) {
return
}
if !assert.Equalf(t, j, shared[key], "goroutine %d iter %d shared[%q]", id, j, key) {
return
}
if !assert.Equalf(t, "from-parent", shared["baseline"],
"goroutine %d iter %d: parent baseline lost", id, j) {
return
}
ctx = next
}
}()
}
wg.Wait()

// Parent context must be unchanged after all the concurrent
// enrichment — the deep-copy fix guarantees independence.
parentData, err := p.GetData(parent)
require.NoError(t, err)
parentShared, ok := parentData["shared"].(map[string]any)
require.True(t, ok)
require.Equal(t, "from-parent", parentShared["baseline"])
require.Len(t, parentShared, 1, "parent.shared must hold only the baseline key; got %v", parentShared)
}