search

ibraheemdev / seize

Fast, efficient, and robust memory reclamation for Rust.
https://docs.rs/seize
MIT License
388 stars 13 forks source link

impl Clone for Collector might be a footgun #28

Open caelunshun opened 6 months ago

caelunshun commented 6 months ago

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.

ibraheemdev commented 6 months ago

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 commented 3 weeks ago

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

ibraheemdev commented 1 week ago

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