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

Restructure cleaning up of the futures in decoupled mode #309

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

Tabrizian
Copy link
Member

Fixes triton-inference-server/server#6270 (comment)

Huge shout out to @rmccorm4 for finding the root cause.

@rmccorm4
Copy link
Contributor

Nice fix! Spoke offline, let's try to make the messages in these blocks also unique_ptr instead of shared_ptr if possible by moving the unique_ptr into the lambda captures, to reduce risk of reintroducing a leak due to long lived reference.

@Tabrizian
Copy link
Member Author

@rmccorm4 I tried using unique_ptr but for some reason it looks like it needs to be a const unique_ptr so it looks like it might need further digging to make it work with unique_ptr.

@rmccorm4
Copy link
Contributor

rmccorm4 commented Oct 10, 2023

@rmccorm4 I tried using unique_ptr but for some reason it looks like it needs to be a const unique_ptr so it looks like it might need further digging to make it work with unique_ptr.

Can you add a ticket to look into it later? I think it would be good to reduce future risk.

Copy link
Contributor

@krishung5 krishung5 left a comment

Choose a reason for hiding this comment

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

Nice work on finding the root cause and fixing the leak!

@Tabrizian
Copy link
Member Author

@rmccorm4 Filed a ticket.

@Tabrizian Tabrizian merged commit a2e8f9b into main Oct 12, 2023
3 checks passed
@Tabrizian Tabrizian deleted the imant-fix-mem-leak branch October 12, 2023 13:42
Tabrizian added a commit that referenced this pull request Oct 12, 2023
* Restructure cleaning up of the futures in decoupled

* Minor improvement
Tabrizian added a commit that referenced this pull request Oct 12, 2023
* Restructure cleaning up of the futures in decoupled

* Minor improvement
kthui added a commit that referenced this pull request Dec 15, 2023
kthui added a commit that referenced this pull request Dec 15, 2023
… of the futures in decoupled mode (#309) (#311)""

This reverts commit 5834064.
zhyncs pushed a commit to zhyncs/python_backend that referenced this pull request Mar 28, 2024
zhyncs added a commit to zhyncs/python_backend that referenced this pull request Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Memory Leak in python backend decouple mode
3 participants