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

Keep reference to k_db abstract and avoid early finalizer call #200

Conversation

jonasmalacofilho
Copy link
Member

In the SQLite CFFI library, connect returns an abstract Neko value, of k_db kind and with a finalizer set. If the k_result request value only keeps a pointer to the db struct, instead of the corresponding k_db value, the finalizer will be called as soon as no other external
references remain to that connection, even if the request itself is still reachable.

The issue could manifest in code like (Haxe):

var rs = Sqlite.open("db.db").request("select * from tbl");
trace(rs.length);

Related: HaxeFoundation/haxe#8728

In the SQLite CFFI library, `connect` returns an abstract Neko value, of
`k_db` kind and with a finalizer set.  If the `k_result` request value
only keeps a pointer to the db struct, instead of the corresponding
`k_db` value, the finalizer will be called as soon as no other external
references remain to that connection, even if the request itself is
still reachable.

The issue could manifest in code like (Haxe):

	var rs = Sqlite.open("db.db").request("select * from tbl");
	trace(rs.length);
@ncannasse
Copy link
Member

ncannasse commented Sep 5, 2019

Should we should check if val_db(r->database) == NULL since the database might have been closed in the meanwhile

@jonasmalacofilho
Copy link
Member Author

@ncannasse maybe, where exactly do you mean?

One thing I noticed is that free_db will set r->done early on, and that cleanly stops result_next. In fact, that's why we had no exception or segfault even though the db had been closed and freed.

@Simn Simn merged commit 8e9ff1e into HaxeFoundation:master Feb 8, 2024
@Simn
Copy link
Member

Simn commented Feb 8, 2024

I was told to merge this!

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