grafana / agent

Vendor-neutral programmable observability pipelines.
https://grafana.com/docs/agent/
Apache License 2.0
1.6k stars 487 forks source link

River: capsules of Go structs always get copied by value #1947

Closed rfratto closed 2 years ago

rfratto commented 2 years ago

The following test will fail:

package value_test 

import (
  "github.com/grafana/agent/pkg/river/internal/value"
  "github.com/stretchr/testify/require"
)

type capsule struct{} 
func (capsule) RiverCapsule() {}

func Test(t *testing.T) {
  var rawValue capsule 
  expect := &rawValue 

  var actual *capsule 
  err := value.Decode(value.Encode(expect), &actual) 
  require.NoError(t, err) 
  require.Equal(t, unsafe.Pointer(expect), unsafe.Pointer(actual)) 
}

Currently, decoding into a pointer always allocates new memory, even if the capsule value being decoded from is also a pointer.

This will break any code which is trying to share memory through capsules (e.g., sending a shared cache to another component). It seems like if both sides of the decode are capsules of struct pointers, the memory should be shared. If either side is not a pointer, then it is fine to do a copy-by-value.

The temporary workaround is to use capsules backed by interface values instead; those never get deferenced and so the memory gets shared properly. Another workaround is to use a capsule struct where every field is a pointer, map, func, or interface; inner fields within a capsule get copied as-is.

rfratto commented 2 years ago

Or maybe the current behavior is fine and just needs to be documented; if you transcode from a *Secret to another *Secret, would the developer expect those to point to the same address of memory? That would seem surprising and prone to race conditions (though a component which exposes a *Secret would also be unusual).