Skip to content

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
@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