Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dtessman opened this issue Jan 15, 2024 · 4 comments
Labels
proposal Enhancement idea or proposal

Comments

@dtessman
Copy link

dtessman commented Jan 15, 2024

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 dtessman added the proposal Enhancement idea or proposal label Jan 15, 2024
@Jarema
Copy link
Member

Jarema commented Jan 15, 2024

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.

@dtessman
Copy link
Author

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.

@Jarema
Copy link
Member

Jarema commented Jan 26, 2024

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.

@dtessman
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Enhancement idea or proposal
Projects
None yet
Development

No branches or pull requests

2 participants