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

Fix memory leaks #708

Conversation

DenisBiryukov91
Copy link
Contributor

@DenisBiryukov91 DenisBiryukov91 commented Jan 31, 2024

Addresses a series of memory leaks detected by valgrind (Fixes #695 ):

  • due to cyclic references in Runtime and its dependents
  • due to cyclic references in Session and its dependents
  • due to cyclic references in Resource and its children
  • due to cyclic reference in Mux->Face->State->Mux
  • due to non-terminated tasks after Session.close()

Valgrind still reports some leaks, but they seem to be due to async_std (no mention of any Zenoh code, and similar "leaks" are also observed when running a simple hello world-type async example).

@Mallets
Copy link
Member

Mallets commented Jan 31, 2024

#566 is expected to land soon. I'd suggest to rebase this PR on #566 and to coordinate with @YuanYuYuan regarding task handling and management.

@DenisBiryukov91 DenisBiryukov91 marked this pull request as draft January 31, 2024 18:26
- kill tasks through task controller

- code clean up

- removed valgrind log

- added documentation

- fix fmt

- Runtime::close() now breaks cyclic references in self.router.tables.tables.root_res
@DenisBiryukov91 DenisBiryukov91 force-pushed the fix/memory-leaks-finalizers branch from a6da9e8 to ae0811e Compare February 1, 2024 15:13
@milyin milyin added the bug Something isn't working label Feb 2, 2024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An attempt of finer grain executor and task management is being proposed in #566 .
Moreover, every time a new crate is added it should be also added in the right order for publication in the CI.

@imstevenpmwork
Copy link
Contributor

@DenisBiryukov91 What's the latest status of this PR? Also, would be great if you can add the timeline to the respective issue

@DenisBiryukov91
Copy link
Contributor Author

@imstevenpmwork This was kind of finished for the main branch at the moment of last commit, but due to transition async-std->Tokyo, task termination part would likely need to be redone, so it is postponed until #566 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leaks cleanup
4 participants