Description

erlend-aasland

opened on Aug 17, 2023

Issue body actions

Originally posted by @vstinner in #108015 (comment):

Ignoring the exception here is a bug.

connection_finalize() clears any exception with PyErr_SetRaisedException(), but pysqlite_connection_close_impl() must not ignore silently error, since here we are talking about a raised Python exception! The function must report if an exception was raised. Then the caller is free to ignore it or not.

I suggest to continue ignoring it in finalize, but then write a separated PR to log the "unraisable exception".

Linked PRs

• gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() and .close() #108084
• [3.12] gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() and .close() (#108084) #108141

Activity

1. erlend-aasland

   self-assigned this

   on Aug 17, 2023
2. erlend-aasland

   added

   topic-sqlite3

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   3.11only security fixesonly security fixes

   3.12only security fixesonly security fixes

   3.13only security fixesonly security fixes

   on Aug 17, 2023
3. erlend-aasland

   added a commit that references this issue on Aug 17, 2023

   pythongh-108083: Don't ignore exceptions in sqlite3.Connection.__init…

   0956795
4. serhiy-storchaka commented on Aug 18, 2023

   

   serhiy-storchaka

   on Aug 18, 2023

   Member

   More actions

   I have not found a test for failing Connection.close(). Would it be possible to write one? Maybe in a separate issue.
5. erlend-aasland

   added 2 commits that reference this issue on Aug 18, 2023

   gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() a…

   fd19509

   [3.12] pythongh-108083: Don't ignore exceptions in sqlite3.Connection…

   488886c
6. erlend-aasland commented on Aug 19, 2023

   

   erlend-aasland

   on Aug 19, 2023

   ContributorAuthor

   More actions

   I have not found a test for failing Connection.close(). Would it be possible to write one? Maybe in a separate issue.

   It should be possible to force the implicit ROLLBACK in .close() to fail using some kind of database lock trick. Perhaps we could use a similar trick as in test_ctx_mgr_rollback_if_commit_failed.
7. Yhg1s

   added a commit that references this issue on Aug 19, 2023

   [3.12] gh-108083: Don't ignore exceptions in sqlite3.Connection.__ini…

   0c21298
8. erlend-aasland commented on Aug 23, 2023

   

   erlend-aasland

   on Aug 23, 2023

   ContributorAuthor

   More actions

   Keeping this open until tests are added.
9. serhiy-storchaka commented on Aug 23, 2023

   

   serhiy-storchaka

   on Aug 23, 2023

   Member

   More actions

   sqlite3_close() can be made failing in many ways, but sqlite3_close_v2() tries hard to return SQLITE_OK, so it may be not so easy to provoke a failure.

   https://www.sqlite.org/c3ref/close.html
10. erlend-aasland commented on Aug 23, 2023

    

    erlend-aasland

    on Aug 23, 2023

    ContributorAuthor

    More actions

    Yes, sqlite3_close_v2() returns SQLITE_OK if it is given a valid database handle.
    sqlite3_close can return an error, but it is hard to see how to be able to trigger that from the constructor; I'm not sure it is possible. We could just as well use sqlite3_close_v2() in the constructor.
11. vstinner commented on Aug 23, 2023

    

    vstinner

    on Aug 23, 2023

    Member

    More actions

    If closing a connection can fail but the sqlite3_close_v2() ignores errors, is there another API to "flush" a connection and reports errors?

    If there is no easy way to trigger an error at exit or to report it, I suggest to just give up and close the issue.
12. erlend-aasland commented on Aug 24, 2023

    

    erlend-aasland

    on Aug 24, 2023

    ContributorAuthor

    More actions

    If there is no easy way to trigger an error at exit or to report it, I suggest to just give up and close the issue.

    I don't think there is; the case is the constructor, between the sqlite3_open_v2 call and the following four goto error jumps. I don't see how we can provoke an error there.

    Closing this as resolved.
13. erlend-aasland

    closed this as completedon Aug 24, 2023
14. vstinner commented on Aug 24, 2023

    

    vstinner

    on Aug 24, 2023

    Member

    More actions

    Thanks, nice sqlite3 enhancement! Ignoring exceptions silently is never a good idea.

Issue actions