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

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

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 17, 2023

Always check the return value of connection_exec_stmt()

…__() and .close()

Always check the return value of connection_exec_stmt()
@vstinner
Copy link
Member

Maybe only change close once you converted sqlite connection to finalizer API.

Maybe start with a first PR to convert dealloc to finalizer API?

@erlend-aasland
Copy link
Contributor Author

Maybe only change close once you converted sqlite connection to finalizer API.

Maybe start with a first PR to convert dealloc to finalizer API?

Yeah, I think I want to land #108015 first.

@erlend-aasland
Copy link
Contributor Author

Maybe start with a first PR to convert dealloc to finalizer API?

Actually, I'll land this first; I want to wait for some external opinions before continuing with the finalizer PR :)

@erlend-aasland
Copy link
Contributor Author

@vstinner, I added the finalizer code from #108015 here. I thought it made for a cleaner shutdown operation. I also made sure we close the database before we free the callback contexts.

There's a few things I'd like to do post this PR, and that is to properly handle sqlite3_close*() errors. I think we should finish this first.

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Show resolved Hide resolved
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, nice fix.

I just left a minor coding style remark, feel free to ignore it.

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

Thanks for the reviews, Serhiy and Victor; it helps to have more eyes, especially for scenarios like this. It is easy to misstep.

@erlend-aasland erlend-aasland enabled auto-merge (squash) August 18, 2023 10:25
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@erlend-aasland erlend-aasland merged commit fd19509 into python:main Aug 18, 2023
22 checks passed
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @erlend-aasland, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker fd195092204aa7fc9f13c5c6d423bc723d0b3520 3.12

@miss-islington
Copy link
Contributor

Sorry, @erlend-aasland, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker fd195092204aa7fc9f13c5c6d423bc723d0b3520 3.11

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Aug 19, 2023
….__init__() and .close() (python#108084)

- Add explanatory comments
- Add return value to connection_close() for propagating errors
- Always check the return value of connection_exec_stmt()
- Assert pre/post state in remove_callbacks()
- Don't log unraisable exceptions in case of interpreter shutdown
- Make sure we're not initialized if reinit fails
- Try to close the database even if ROLLBACK fails

(cherry picked from commit fd19509)

Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot
Copy link

GH-108141 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Aug 19, 2023
@erlend-aasland erlend-aasland deleted the sqlite/do-not-ignore-exceptions branch August 19, 2023 16:01
@erlend-aasland erlend-aasland removed the needs backport to 3.11 only security fixes label Aug 19, 2023
@erlend-aasland
Copy link
Contributor Author

FYI: connection_exec_stmt is not in 3.11.

Yhg1s pushed a commit that referenced this pull request Aug 19, 2023
…t__() and .close() (#108084) (#108141)

- Add explanatory comments
- Add return value to connection_close() for propagating errors
- Always check the return value of connection_exec_stmt()
- Assert pre/post state in remove_callbacks()
- Don't log unraisable exceptions in case of interpreter shutdown
- Make sure we're not initialized if reinit fails
- Try to close the database even if ROLLBACK fails

(cherry picked from commit fd19509)

Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
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.

None yet

6 participants