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 pinned memory leaks if queries are not disposed. #375

Merged
merged 4 commits into from
Feb 11, 2024

Conversation

teo-tsirpanis
Copy link
Member

@teo-tsirpanis teo-tsirpanis commented Feb 3, 2024

SC-39730

To pass query buffers to the Core, the C# API has to pin them to keep them alive and prevent the GC from moving them, and they get unpinned when the query gets Disposed. However, if the user forgets to dispose the query, the buffers do not get unpinned, resulting in a memory leak. The native tiledb_query_t* still gets freed because it is managed by a SafeHandle which has a finalizer.

This PR fixes this leak by moving the query buffer management logic from Query to QueryHandle. This means that if the Query does not get disposed, its QueryHandle will also not get disposed, its finalizer will run and both free the tiledb_query_t* and unpin the buffers.

I added a test that verifies that the GC frees the buffers of an undisposed query.

I have considered alternative ways to fix this, like putting finalizers on the Query or BufferHandle classes, but we can't do it because finalizers execute in a non-deterministic order and we must free the tiledb_query_t* first before unpinning the buffers (and adding a finalizer to BufferHandle will have performance implications since we are creating more than one finalizable object per query).

Copy link

This pull request has been linked to Shortcut Story #39730: Query objects leak pinned GC handles if not disposed..

@teo-tsirpanis teo-tsirpanis merged commit d131964 into TileDB-Inc:main Feb 11, 2024
4 checks passed
@teo-tsirpanis teo-tsirpanis deleted the buffer-leak-fix branch February 11, 2024 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants