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.

Sign up for GitHub

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

Jump to bottom

Fix Python Shm Client Leak #614

Merged
merged 3 commits into from
May 7, 2024
Merged

Fix Python Shm Client Leak #614

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8024e1
Delete dynamic shm struct
fpetrini15 Apr 29, 2024
5c4e1f6
Edge cases
fpetrini15 May 3, 2024
3fb4551
Add comment
fpetrini15 May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/python/library/tritonclient/utils/shared_memory/shared_memory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ SharedMemoryRegionDestroy(void* shm_handle)
return -5;
}

delete handle;
Copy link

@whoisj whoisj May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

given the return statement above, this may not be enough.

does the failure to shm_unlink mean that handle is invalid and should not be deleted?

seems to me that the memory is still allocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch J! No, in this case handle refers to a dynamically allocated instance of our internal struct definition as opposed to an OS handle. We are doing best-effort cleanup with munmap and shm_unlink, but we have full control to deallocate handle.

Testing various cases with the new change also reminded me of a minor logical error within the python client code that we should clean up here as well. Note, that I elected to use a local smart pointer to do cleanup to avoid having to write three proximal deletes, however, I can switch to using an explicit delete to match the allocation syntax.


return 0;
}

Expand Down
Loading