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

Use sqlite_close_v2() #288

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Use sqlite_close_v2() #288

merged 1 commit into from
Dec 7, 2023

Conversation

rinzevdwalAlfen
Copy link
Contributor

Use sqlite3_close_v2() instead of sqlite3_close(). Since we use the close in a destructor and and a destructor shoudn't fail. We should use the sqlite3_close_v2() instead since it will automatically deallocate the database connection after all prepared statements are finalized, all BLOB handles are closed, and all backups have finished.

sqlite_close documentation

Signed-off-by: Rinze van der Wal <R.vanderWal@alfen.com>
Copy link
Contributor

@valentin-dimov valentin-dimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my understanding, this change would mean that we won't get error prints from the destructor, even if the database is busy - however, sqlite_close_v2 will not wait for in-progress work to finish, it will simply arrange for the connection to be cleaned up whenever the work is done. If we call close_connection right before shutting down (which is, I assume, the main use-case), it seems to me that this could still lead to the process exiting with a dirty connection, since we never actually wait for the database connection to be destroyed.

@rinzevdwalAlfen
Copy link
Contributor Author

The documentation states

If sqlite3_close_v2() is called with unfinalized prepared statements, unclosed BLOB handlers, and/or unfinished sqlite3_backups, it returns SQLITE_OK regardless, but instead of deallocating the database connection immediately, it marks the database connection as an unusable "zombie" and makes arrangements to automatically deallocate the database connection after all prepared statements are finalized, all BLOB handles are closed, and all backups have finished. The sqlite3_close_v2() interface is intended for use with host languages that are garbage collected, and where the order in which destructors are called is arbitrary.

So I am not sure if you are correct when stating that the sqlite_close_v2 will not wait for in-progress work to finish. Since it will finalize all prepared statements.

On the other hand with sqlite_close as we do now. The call will return SQLITE_BUSY and not close the connection.

@valentin-dimov
Copy link
Contributor

I'm not particularly familiar with sqlite3, but reading the doc page, I'm not sure that when it says the function "makes arrangements to automatically deallocate the database connection after all prepared statements are finalized [...]", this means the function blocks until that work is finished. Is it possible it'll return early and the work continues in the background?

Copy link
Contributor

@valentin-dimov valentin-dimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following our discussion: looks good, noting that we still need to look into why we were getting busy errors before, as those are still unexpected.

Copy link
Contributor

@Dominik-K Dominik-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DatabaseHandler deconstructor used the sqlite3_close_v2() until one month ago until PR

So it's ok to get back to this state.

However, it would be good to reflect this in OCPP 1.6, too. Seems that the DatabaseHandler itself could be a generic one.

@rinzevdwalAlfen rinzevdwalAlfen merged commit 01c3a6b into main Dec 7, 2023
3 checks passed
@rinzevdwalAlfen rinzevdwalAlfen deleted the rw-close-sqlite branch December 7, 2023 15:17
@Dominik-K
Copy link
Contributor

Dominik-K commented Dec 19, 2023

However, it would be good to reflect this in OCPP 1.6, too. Seems that the DatabaseHandler itself could be a generic one.

Tracked in

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