nats.rs icon indicating copy to clipboard operation
nats.rs copied to clipboard

async_nats: Endpoint: Remove endpoint from list of stats endpoints when dropped.

Open dtessman opened this issue 5 months ago • 4 comments

Proposed change

Drop endpoint info and stats when endpoint dropped.

Use case

I am adding and removing endpoints dynamically from a service. When I wish to remove an endpoint, I stop the endpoint and then drop the instance. When stopped, the endpoint becomes unavailable. However, the info and stats remain, even if the endpoint instance is dropped. This can be confusing for discovery and acts as a memory leak. On my local fork, I am removing the endpoint from the services stats whenever the endpoint itself is dropped. Not sure If I am missing anything, but it seems to work for my use-case.

Contribution

Yes as follows:

impl Drop for Endpoint {
    /// Removes the [Endpoint] from list of stats endpoints.
    fn drop(&mut self) {
        self.stats.lock().unwrap().endpoints.remove(&self.endpoint);
    }
}

dtessman avatar Jan 15 '24 01:01 dtessman

Hey, thanks for filling the issue!

I understand the concern about the memory, however I also see the other side: someone might want to track the stats of the whole service throughout its lifetime.

A quick thought - we could make the behaviour you propose a default one, and introduce the config option to retain the stats. I will also check how other clients handle this case to make sure all of them re aligned.

Will get back to this issue later today.

Jarema avatar Jan 15 '24 07:01 Jarema

One could also add the ability to reset stats for a specific endpoint and if the endpoint is closed, stats are removed completely. Though I do think the drop is important as a kind of default cleanup behavior as you mentioned above.

dtessman avatar Jan 16 '24 22:01 dtessman

I checked the other clients and the ADR, and only Rust clients allows for removing endpoints, by usage of Rust mechanism of dropping.

Need to think how to move this one forward to not introduce client discrepancies.

Jarema avatar Jan 26 '24 14:01 Jarema

Thanks for looking into it! Am I correct in that the code that I posted above is all I need to worry about on my fork? There is no other hidden structures that I am missing in the drop?

dtessman avatar Jan 30 '24 03:01 dtessman