GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() and .close() by erlend-aasland · Pull Request #108084 · python/cpython

Conversation

erlend-aasland commented Aug 17, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Always check the return value of connection_exec_stmt()

Issue: sqlite3: some code paths ignore exceptions #108083

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


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

…__() and .close()

Always check the return value of connection_exec_stmt()

erlend-aasland requested a review from berkerpeksag as a code owner

August 17, 2023 11:01

erlend-aasland added needs backport to 3.11

only security fixes

needs backport to 3.12 only security fixes labels

Aug 17, 2023

erlend-aasland requested a review from vstinner

August 17, 2023 11:01

bedevere-bot mentioned this pull request

Aug 17, 2023

sqlite3: some code paths ignore exceptions #108083

Closed

bedevere-bot added the awaiting core review label

Aug 17, 2023

erlend-aasland mentioned this pull request

Aug 17, 2023

gh-105539: Emit ResourceWarning if sqlite3 database is not closed explicitly #108015

Merged

erlend-aasland commented

Aug 17, 2023

View reviewed changes

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Aug 17, 2023

Copy link

Copy Markdown

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented Aug 17, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Cast

a5f6454

serhiy-storchaka reviewed

Aug 17, 2023

View reviewed changes

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented Aug 17, 2023

Copy link

Copy Markdown

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 :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 3 commits

August 17, 2023 21:16


          Pull in main

367f192


          Address review: let connection_close() return a value and the caller …

7b3d032

…decide what to do


          Don't log unraisable exceptions in case of interp shutdown

379e64e

erlend-aasland commented Aug 17, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reviewed

Aug 17, 2023

View reviewed changes

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Pull in main

3905a88

serhiy-storchaka reviewed

Aug 18, 2023

View reviewed changes

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added 4 commits

August 18, 2023 09:38


          Assert pre/post state in remove_callbacks()

b2d49f3


          Make sure we're not initialized if reinit fails

0d4eb8b


          Try to close the database even if ROLLBACK fails

863f5b7


          Add comment

ef5418a

vstinner approved these changes

Aug 18, 2023

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting merge and removed awaiting core review labels

Aug 18, 2023

erlend-aasland added 2 commits

August 18, 2023 12:19


          Pull in main

b1b6d24


          Reduce indent

5d308aa

erlend-aasland commented Aug 18, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland enabled auto-merge (squash)

August 18, 2023 10:25

kumaraditya303 reviewed

Aug 18, 2023

View reviewed changes

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka approved these changes

Aug 18, 2023

View reviewed changes

Comment thread

Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Also assert usage after set authorizer

8d57138

Co-authored-by: Serhiy Storchaka <[email protected]>

erlend-aasland merged commit fd19509 into python:main

Aug 18, 2023

miss-islington commented Aug 18, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Aug 18, 2023

miss-islington commented Aug 18, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington assigned erlend-aasland

Aug 18, 2023

miss-islington commented Aug 18, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request

Aug 19, 2023


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

488886c

….__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 <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot commented Aug 19, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.12 only security fixes label

Aug 19, 2023

erlend-aasland deleted the sqlite/do-not-ignore-exceptions branch

August 19, 2023 16:01

erlend-aasland removed the needs backport to 3.11 only security fixes label

Aug 19, 2023

erlend-aasland commented Aug 19, 2023

Copy link

Copy Markdown

Contributor Author

FYI: connection_exec_stmt is not in 3.11.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Yhg1s pushed a commit that referenced this pull request

Aug 19, 2023


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

0c21298

…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 <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>

Reviewers

vstinner

vstinner approved these changes

kumaraditya303

kumaraditya303 left review comments

serhiy-storchaka

serhiy-storchaka approved these changes

berkerpeksag

Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

erlend-aasland

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

erlend-aasland commented Aug 17, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vstinner commented Aug 17, 2023

Uh oh!

erlend-aasland commented Aug 17, 2023

Uh oh!

Uh oh!

erlend-aasland commented Aug 17, 2023

Uh oh!

erlend-aasland commented Aug 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Aug 18, 2023

Uh oh!

Uh oh!

Uh oh!

miss-islington commented Aug 18, 2023

Uh oh!

miss-islington commented Aug 18, 2023

Uh oh!

miss-islington commented Aug 18, 2023

Uh oh!

bedevere-bot commented Aug 19, 2023

Uh oh!

erlend-aasland commented Aug 19, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erlend-aasland commented Aug 17, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.