metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.08k stars 148 forks source link

`set_boxed_recorder` leaks the box if the recorder is already set #409

Closed lilyball closed 8 months ago

lilyball commented 9 months ago

If I'm calling set_boxed_recorder after a recorder has already been set, it returns an error, but it also leaks the boxed recorder. This is causing asan to flag leaks in my test suite where I'm using metrics_util::debugging::DebuggingRecorder::per_thread().install() in each test case.

tobz commented 9 months ago

Hmm, yeah, that is unfortunate.

We could probably adjust the recorder set logic to reconstitute the Box<T> if we hit the error path, and allow it to be properly dropped.

lilyball commented 9 months ago

It looks like you can pass the reference into Box::from_raw() to get the original Box back, the only downside is it's unsafe.

tobz commented 9 months ago

Yeah, that's true. I was thinking I might bundle the logic into RecorderOnceCell so that we can pass it an enum of either a static reference, or a boxed recorder, and depending on which it gets, it implements the logic to drop the boxed variant if it fails to set the recorder.

Anyways, regardless of the implementation approach, it does seem like we can do this without too much fuss. Not entirely sure when I'll get to it, though.