2shady4u / godot-sqlite

GDExtension wrapper for SQLite (Godot 4.x+)
MIT License
850 stars 76 forks source link

Error message about trying to close a db that's not open #169

Closed scotmcp closed 5 months ago

scotmcp commented 5 months ago

Environment:

Issue description:

On opening the sqlite database, getting error: GDSQLite Error: Can't close database if connection is not open!

I am pasting the entirety of my database class as it exists now. I have searched my entire project for errant db.close() statements, and there are none.

Steps to reproduce: You have to run my project, which is far too big to paste here, however I did paste my database class in. I cannot find logic that would cause a db.close() to happen without an instance being opened first.

Additionally, it sometimes produces there error open the very first openning of the database (giving the message that it can't close the database that isn't open)

Minimal reproduction project: database.db.tar.gz

Additional context image

image

2shady4u commented 5 months ago

Hello! I will investigate as soon as possible. There is a chance this might be caused by the close_db()-method call in the plugin's destructor. In which case the issue is caused by me 🙈

2shady4u commented 5 months ago

@scotmcp I have pushed a small fix since that addresses the issue. Updated binaries will be part of the next release, but you can already download the new binaries here: https://github.com/2shady4u/godot-sqlite/actions/runs/8070008064

Could you confirm that the issue is fixed when you use the new binary? 😃

scotmcp commented 5 months ago

I'll check it out later, looks like it's all still building.

EDIT: Good lord, it's taking forever....what sort of Frankenstein have you built? 😃

Also question, i checked out the patch....how does checking if the db is closed during destruction solve the problem? I am not much of a C++ dev, so I just don't know.

scotmcp commented 5 months ago

Well regardless of whether I know what I am talking about or not, the new libraries seem to work great for Linux....I haven't tested in Windows, but I assume it will work there too.

2shady4u commented 5 months ago

I'll check it out later, looks like it's all still building.

EDIT: Good lord, it's taking forever....what sort of Frankenstein have you built? 😃

Also question, i checked out the patch....how does checking if the db is closed during destruction solve the problem? I am not much of a C++ dev, so I just don't know.

The destructor automatically gets called by Godot when the SQLite object goes out of scope. Normally, before going out of scope, the user closes the database connection by calling 'close_db()'-method him/herself. BUT!!! In the destructor method (which automatically gets called) the 'close_db()'-method was called a second time!

Why close the database connection when the object goes out of scope? A user might forget to close the database connection. Not closing the connection is bad because this leads to a memory leak.

How to fix? A possible way to fix this issue is to check in the destructor if the database connection is still open or not.

scotmcp commented 5 months ago

I guess what I don't understand is...why fixing the destructor solves the problem of trying to close the database when what I am doing is opening the database, does the constructor look for an instance to destroy first, like a singleton?

2shady4u commented 5 months ago

I guess what I don't understand is...why fixing the destructor solves the problem of trying to close the database when what I am doing is opening the database, does the constructor look for an instance to destroy first, like a singleton?

I am not 100% sure, but I think the editor (in some way & at some point) instances the SQLite class to get access to its internal methods for documentation and autocompletion purposes. This instance then gets destroyed by the editor, resulting in the 'close_db()'-method call which results in the error you are experiencing.

scotmcp commented 5 months ago

THAT makes sense :)

Thank you!!