seize icon indicating copy to clipboard operation
seize copied to clipboard

impl Clone for Collector might be a footgun

Open caelunshun opened this issue 1 year ago • 1 comments

I recently tracked down a bug in some code caused by my incorrect assumption about Collector::clone: I assumed it would behave like an Arc in maintaining a reference to the underlying data structure, which (I thought) is a common pattern in Rust crates dealing with concurrency. This, of course, was incorrect and led to objects being mixed between separate collectors.

I see now that the docs for Collector::clone mention this behavior, but rustdoc doesn't do a great job of showing docs for trait implementations. I wondered if it would be less likely to cause confusion if the cloning behavior was in a differently named method, e.g. clone_config.

This is just a suggestion. Maybe I'm the only one who would make this assumption about clone, not sure.

caelunshun avatar May 22 '24 02:05 caelunshun

Yes, I completely agree. I was actually meaning to change this a while ago but it fell off my radar. I think we should probably just remove the Clone implementation, and add with_config(self, config: Config) and config(&self) -> Config methods that can be used to easily recreate collector configuration (where Config holds the current epoch_frequency and batch_size settings).

ibraheemdev avatar May 23 '24 03:05 ibraheemdev

Removed in https://github.com/ibraheemdev/seize/commit/97ef5794916a1e5afdefdebaac5fac68e1244466.

ibraheemdev avatar Nov 07 '24 23:11 ibraheemdev

Reopening this until the fix releases, it's not part of the latest release as it is a breaking change.

ibraheemdev avatar Nov 27 '24 19:11 ibraheemdev