Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedAug 17, 2023•
edited by bedevere-bot
Loading

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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

@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 dopost this PR, and that is to properly handlesqlite3_close*() errors. I think we should finish this first.

Copy link
Member

@vstinnervstinner 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.

@erlend-aasland
Copy link
ContributorAuthor

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

@erlend-aaslanderlend-aaslandenabled auto-merge (squash)August 18, 2023 10:25
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@erlend-aaslanderlend-aasland merged commitfd19509 intopython:mainAug 18, 2023
@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 to3.12 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker fd195092204aa7fc9f13c5c6d423bc723d0b3520 3.12

@miss-islington
Copy link
Contributor

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

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull requestAug 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 commitfd19509)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 the3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12only security fixes labelAug 19, 2023
@erlend-aaslanderlend-aasland deleted the sqlite/do-not-ignore-exceptions branchAugust 19, 2023 16:01
@erlend-aaslanderlend-aasland removed the needs backport to 3.11only security fixes labelAug 19, 2023
@erlend-aasland
Copy link
ContributorAuthor

FYI:connection_exec_stmt is not in 3.11.

Yhg1s pushed a commit that referenced this pull requestAug 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 commitfd19509)Co-authored-by: Victor Stinner <vstinner@python.org>Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner approved these changes

@kumaraditya303kumaraditya303kumaraditya303 left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksagberkerpeksag is a code owner

Assignees

@erlend-aaslanderlend-aasland

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@erlend-aasland@vstinner@miss-islington@bedevere-bot@serhiy-storchaka@kumaraditya303
[8]ページ先頭
©2009-2025 Movatter.jp