Skip to content

Commit e1b4f56

Browse files
authored
fix(ruby): keep OpenTelemetry FFI config reference (#198)
* fix(ruby): keep OpenTelemetry FFI config buffers alive across init build_config stored only the raw addresses of the nested traces/metrics FFI struct wrappers and their endpoint string buffers into the config struct, then let those wrapper objects go out of scope. The endpoint MemoryPointers are retained by the wrapper struct instances, not by the config struct (which holds only their backing addresses), so GC between build_config returning and Bindings.init_open_telemetry running could free the endpoint buffers -- leaving config->traces/metrics->endpoint dangling when the native layer reads them (use-after-free). build_config now returns [config_struct, keep_alive] where keep_alive pins the nested struct wrappers and endpoint buffers; init stashes it in @config_refs for the process lifetime (init is one-time), guaranteeing the memory outlives the native call. Mirrors the buffer-pinning pattern in build_command_args. Related to #179. Signed-off-by: Alex Le <alex.le@improving.com> * refactor(ruby): release OTel config buffers after init instead of pinning for process lifetime The native init_open_telemetry copies the endpoint strings synchronously (CStr::from_ptr(...).to_string()) and retains no pointers, so the config buffers only need to outlive the single FFI call. Hold keep_alive as a local across the call and clear it afterwards rather than stashing it in a process-lifetime @config_refs ivar. Also documents build_config's two-element return and its buffer-pinning contract. Related to #179. Signed-off-by: Alex Le <alex.le@improving.com> * docs: updated comments Signed-off-by: Alex Le <alex.le@improving.com> --------- Signed-off-by: Alex Le <alex.le@improving.com>
1 parent 7121a4c commit e1b4f56

1 file changed

Lines changed: 26 additions & 5 deletions

File tree

lib/valkey/opentelemetry.rb

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,12 @@ def init(traces: nil, metrics: nil, flush_interval_ms: nil, parent_span_context_
9898
end
9999

100100
# Build the configuration
101-
config = build_config(traces, metrics, flush_interval_ms)
101+
# keep_alive needs to be referenced so not to be GC'd before init_open_telemetry
102+
# TODO: Refactor per https://github.com/valkey-io/valkey-glide-ruby/issues/179
103+
config, keep_alive = build_config(traces, metrics, flush_interval_ms)
102104

103-
# Call the FFI function
104105
error_ptr = Bindings.init_open_telemetry(config)
106+
keep_alive.clear # Needed to avoid linter warning.
105107

106108
unless error_ptr.null?
107109
error_msg = error_ptr.read_string
@@ -223,15 +225,30 @@ def validate_parent_span_context!(ctx)
223225
raise ArgumentError, "tracestate must be a String or nil, got: #{tracestate.class}"
224226
end
225227

228+
# Build the native OpenTelemetry configuration struct from the given options.
229+
#
230+
# @param traces [Hash, nil] Traces configuration (see {init})
231+
# @param metrics [Hash, nil] Metrics configuration (see {init})
232+
# @param flush_interval_ms [Integer, nil] Flush interval in milliseconds
233+
#
234+
# @return [Array(Bindings::OpenTelemetryConfig, Array)] a two-element array of:
235+
# - the `config` struct to pass to `Bindings.init_open_telemetry`
236+
# - a `keep_alive` array containing references to values in `config`.
237+
# This is so the caller can keep them alive in their own scope, otherwise
238+
# they may be garbage collected before use.
239+
# TODO: Refactor per https://github.com/valkey-io/valkey-glide-ruby/issues/179
226240
def build_config(traces, metrics, flush_interval_ms)
227241
config_struct = Bindings::OpenTelemetryConfig.new
228242

243+
keep_alive = [config_struct]
244+
229245
# Configure traces if provided
230246
if traces
231247
validate_endpoint!(traces[:endpoint], "traces")
232248

233249
traces_struct = Bindings::OpenTelemetryTracesConfig.new
234-
traces_struct[:endpoint] = FFI::MemoryPointer.from_string(traces[:endpoint])
250+
endpoint_ptr = FFI::MemoryPointer.from_string(traces[:endpoint])
251+
traces_struct[:endpoint] = endpoint_ptr
235252

236253
if traces[:sample_percentage]
237254
traces_struct[:has_sample_percentage] = true
@@ -242,6 +259,8 @@ def build_config(traces, metrics, flush_interval_ms)
242259
end
243260

244261
config_struct[:traces] = traces_struct.pointer
262+
# Pin the wrapper (owns the endpoint ref) and the endpoint buffer.
263+
keep_alive.push(traces_struct, endpoint_ptr)
245264
else
246265
config_struct[:traces] = FFI::Pointer::NULL
247266
end
@@ -251,8 +270,10 @@ def build_config(traces, metrics, flush_interval_ms)
251270
validate_endpoint!(metrics[:endpoint], "metrics")
252271

253272
metrics_struct = Bindings::OpenTelemetryMetricsConfig.new
254-
metrics_struct[:endpoint] = FFI::MemoryPointer.from_string(metrics[:endpoint])
273+
endpoint_ptr = FFI::MemoryPointer.from_string(metrics[:endpoint])
274+
metrics_struct[:endpoint] = endpoint_ptr
255275
config_struct[:metrics] = metrics_struct.pointer
276+
keep_alive.push(metrics_struct, endpoint_ptr)
256277
else
257278
config_struct[:metrics] = FFI::Pointer::NULL
258279
end
@@ -266,7 +287,7 @@ def build_config(traces, metrics, flush_interval_ms)
266287
config_struct[:flush_interval_ms] = 5000 # Default
267288
end
268289

269-
config_struct
290+
[config_struct, keep_alive]
270291
end
271292

272293
def validate_endpoint!(endpoint, type)

0 commit comments

Comments
 (0)