GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-27334: roll back transaction if sqlite3 context manager fails to commit by erlend-aasland · Pull Request #26202 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented May 17, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue27334

Sorry, something went wrong.

Uh oh!

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

All reactions
Erlend E. Aasland and others added 2 commits May 18, 2021 00:31
Erlend E. Aasland @berkerpeksag
bpo-27334: sqlite3 context manager now rolls back if commit fails
a1361e6
Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <[email protected]>
Erlend E. Aasland
Add NEWS
7111bba
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner May 17, 2021 22:38
@bedevere-bot bedevere-bot added the awaiting review label May 17, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 17, 2021
@erlend-aasland

erlend-aasland commented May 17, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@berkerpeksag, a couple of comments accompanying the change:

  1. Since the test calls for another process (I can't reproduce it using threads), I added a new test class MultiprocessTests for this. If there's a better way, shout out :)
  2. AFAICS, the test should be able to clean up both the test database file and the child process, regardless of where it may fail. The first input() in the child process should take care of any race conditions.
  3. I swapped method_name with a flag for performance reasons; it's easier to test for a flag when result == NULL
  4. Because of 3., it was easier to just call the commit/rollback functions directly, iso. going through a PyObject_Call; hope that's ok. As a side effect, __exit__ should now be a tiny bit faster.
  5. The test code could be a little bit prettier using textwrap.dedent, but I decided not to use it, in order to keep the number of imports to a minimum.
All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 17, 2021

Copy link
Copy Markdown
Contributor Author

(cc. @pablogsal)

All reactions

Sorry, something went wrong.

Uh oh!

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

Erlend E. Aasland added 2 commits May 18, 2021 01:03
Erlend E. Aasland
Add explanatory comments to test
0b63628
Erlend E. Aasland
PEP 8
fce98f1
@lciti

lciti commented May 18, 2021

Copy link
Copy Markdown

I've taken the liberty to create a PR based on your patch, Luca. Berker's comments have been addressed in the PR.

Thanks for creating the PR. Sorry for not doing it myself, I almost forgot about the whole thing. It's good that a fix is eventually making its way into the code.

erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 18, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for creating the PR. Sorry for not doing it myself, I almost forgot about the whole thing. It's good that a fix is eventually making its way into the code.

Thanks for reporting, providing a reproducer, and proposing a fix! :) I've credited you in the NEWS entry and in the commit message.

lciti reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland commented May 19, 2021
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

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

pablogsal
pablogsal reviewed May 19, 2021
View reviewed changes
Comment thread Lib/sqlite3/test/dbapi.py Resolved

Uh oh!

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

pablogsal
pablogsal reviewed May 19, 2021
View reviewed changes
Comment thread Lib/sqlite3/test/dbapi.py Resolved

Uh oh!

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

Erlend E. Aasland added 7 commits May 20, 2021 00:34
Erlend E. Aasland
Prevent accidentally clearing the current exception
3edde50
Erlend E. Aasland
... and clean up
99c4157
Erlend E. Aasland
Use test.support.SHORT_TIMEOUT to wait for sub-process
581bc44
Erlend E. Aasland
Adjust comment
31f43fa
Erlend E. Aasland
Make rollback thread friendly, and use _pysqlite_seterror to check th…
a66828b
…e return value
Erlend E. Aasland
Remove unused variable
cdb0d50
Erlend E. Aasland
Dedent test script using if 1 trick
e5fdaf7
pablogsal
pablogsal reviewed May 25, 2021
View reviewed changes
Comment thread Lib/sqlite3/test/dbapi.py Resolved

Uh oh!

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

Erlend E. Aasland
Use test.support.TIMEOUT to adjust connection timeout
f67591c
@erlend-aasland
erlend-aasland requested a review from pablogsal June 2, 2021 09:19
vstinner
vstinner reviewed Jun 2, 2021
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

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

vstinner
vstinner reviewed Jun 2, 2021
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2021-05-18-00-17-21.bpo-27334.32EJZi.rst Resolved

Uh oh!

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

Erlend E. Aasland
Address review: make news entry less low level-ish
4e654d0
@vstinner

vstinner commented Jun 2, 2021

Copy link
Copy Markdown
Member

You can try _PyErr_ChainExceptions() to chain exceptions.

Example:

static PyObject *
_io_FileIO_close_impl(fileio *self)
/*[clinic end generated code: output=7737a319ef3bad0b input=f35231760d54a522]*/
{
    PyObject *res;
    PyObject *exc, *val, *tb;
    int rc;
    _Py_IDENTIFIER(close);
    res = _PyObject_CallMethodIdOneArg((PyObject*)&PyRawIOBase_Type,
                                       &PyId_close, (PyObject *)self);
    if (!self->closefd) {
        self->fd = -1;
        return res;
    }
    if (res == NULL)
        PyErr_Fetch(&exc, &val, &tb);
    if (self->finalizing) {
        PyObject *r = fileio_dealloc_warn(self, (PyObject *) self);
        if (r)
            Py_DECREF(r);
        else
            PyErr_Clear();
    }
    rc = internal_close(self);
    if (res == NULL)
        _PyErr_ChainExceptions(exc, val, tb);
    if (rc < 0)
        Py_CLEAR(res);
    return res;
}
All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 2, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

You can try _PyErr_ChainExceptions() to chain exceptions.

Thanks! I'll have a look at this after #26462 is resolved :)

All reactions

Sorry, something went wrong.

Uh oh!

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

Erlend E. Aasland
Chain exceptions if rollback also fails
98d3ada
@pablogsal

pablogsal commented Aug 25, 2021

Copy link
Copy Markdown
Member

@erlend-aasland Does this need backports? If not, please close the issue :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland mentioned this pull request Aug 25, 2021
bpo-16379: expose SQLite error codes and error names in sqlite3 #27786
Merged
@erlend-aasland
erlend-aasland deleted the sqlite-ctx-rollback branch August 25, 2021 11:13
@erlend-aasland

erlend-aasland commented Aug 25, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

It's a bugfix, so I'd backport to 3.10 and 3.9. (I suspect the backports must be done manually)

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

Thank you so much for reviewing, Pablo and Victor!

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland added needs backport to 3.9 labels Aug 25, 2021
@miss-islington

miss-islington commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

Sorry, @erlend-aasland and @pablogsal, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 7ecd3425d45a37efbc745788dfe21df0286c785a 3.9

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

Sorry @erlend-aasland and @pablogsal, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 7ecd3425d45a37efbc745788dfe21df0286c785a 3.10

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington miss-islington assigned pablogsal Aug 25, 2021
@erlend-aasland erlend-aasland mentioned this pull request Aug 25, 2021
bpo-27334: Fix ref. leak introduced by GH-26202 #27942
Merged
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Aug 25, 2021
@bedevere-bot

bedevere-bot commented Aug 25, 2021

Copy link
Copy Markdown

GH-27943 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Aug 25, 2021
Erlend Egeberg Aasland
[3.10] bpo-27334: roll back transaction if sqlite3 context manager fa…
3978e28
…ils to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <[email protected]>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot

bedevere-bot commented Aug 25, 2021

Copy link
Copy Markdown

GH-27944 is a backport of this pull request to the 3.9 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Aug 25, 2021
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Aug 25, 2021
Erlend Egeberg Aasland
[3.9] bpo-27334: roll back transaction if sqlite3 context manager fai…
303d132
…ls to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <[email protected]>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
ambv pushed a commit that referenced this pull request Aug 25, 2021
Erlend Egeberg Aasland
bpo-27334: Fix reference leak introduced by GH-26202 (GH-27942)
a3c11ce
pablogsal pushed a commit that referenced this pull request Aug 25, 2021
Erlend Egeberg Aasland
[3.9] bpo-27334: roll back transaction if sqlite3 context manager fai…
52702e8
…ls to commit (GH-26202) (GH-27944)
pablogsal pushed a commit that referenced this pull request Aug 28, 2021
Erlend Egeberg Aasland
[3.10] bpo-27334: roll back transaction if sqlite3 context manager fa…
2a80893
…ils to commit (GH-26202) (GH-27943)
@erlend-aasland erlend-aasland mentioned this pull request Nov 27, 2022
It looks like the following does the job: ghaering/pysqlite#143
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Jun 3, 2026
Perf/reuse sqlite conn Abhigyan-Shekhar/Waggle-mcp#239
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jun 11, 2026
Fix four robustness bugs from the app review jss367/vireo#970
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@vstinner vstinner Awaiting requested review from vstinner
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

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

Assignees

@pablogsal pablogsal

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.

7 participants

@erlend-aasland @lciti @vstinner @pablogsal @miss-islington @bedevere-bot @the-knights-who-say-ni
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.