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

Memory leak fixes in the connector #154

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

taniabogatsch
Copy link
Collaborator

This PR builds on the work of #151. See also my comment here.

Changes in this PR:

  • the Connector now exposes its Close method (done by @levakin). This PR contains the respective commits of refactor Connector to expose Close method #151.
    • explicitly calling c.(io.Closer).Close() is crucial to avoid memory leaks in the database object
    • exposing Close simplifies closing the Connector for users, reducing the risk of leaks and increasing usability
  • maybe this is a lack in my understanding of Go, but I changed db *C.duckdb_database to db C.duckdb_database in the Connector because it is already a pointer (typedef struct _duckdb_database {void *__db;} * duckdb_database;). Or was there a reason to store it as a pointer?
  • I added C.duckdb_destroy_config(&config) to more places to avoid leaks
  • The following two places were leaking, as they never freed the C.CString, to the best of my knowledge
    • C.duckdb_set_config(config, C.CString("duckdb_api"), C.CString("go"))
    • C.duckdb_set_config(config, C.CString(k), C.CString(v[0]))
    • @levakin added setConfig, and I added explicit calls to C.free
  • we do not have to keep the config-object alive after opening a DuckDB database

cc @marcboeker @levakin

@taniabogatsch
Copy link
Collaborator Author

taniabogatsch commented Jan 26, 2024

This is an update to the main PR message concerning my most recent commit.

  • A fix to prevent func (s *stmt) execute(ctx context.Context, args []driver.NamedValue) (*C.duckdb_result, error) from leaking memory on context errors (missing C.duckdb_destroy_result(&res)).
  • A minor fix that prevents TestTypeNamesAndScanTypes from leaking memory.
  • I added a section to the README.md that addresses memory allocations in go-duckdb. It explains why calling Close is crucial in go-duckdb to avoid memory leaks. While adding that section, I also expanded the readme file to make coding patterns more explicit.
  • Some tidying

Note: there are still a few leaks in the appender, which @maiadegraaf addresses in #150.

@marcboeker marcboeker merged commit f7007ad into marcboeker:main Feb 1, 2024
3 checks passed
@marcboeker
Copy link
Owner

Thanks @taniabogatsch

@taniabogatsch taniabogatsch deleted the fix-connector branch February 2, 2024 09:10
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.

3 participants