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

bpo-27334: roll back transaction if sqlite3 context manager fails to commit #26202

Merged
merged 26 commits into from Aug 25, 2021

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented May 17, 2021

https://bugs.python.org/issue27334

erlend-aasland and others added 2 commits May 17, 2021
Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented May 17, 2021

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

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented May 17, 2021

(cc. @pablogsal)

@lciti
Copy link

@lciti lciti commented May 18, 2021

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
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented May 18, 2021

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.

Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Lib/sqlite3/test/dbapi.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/dbapi.py Outdated Show resolved Hide resolved
Lib/sqlite3/test/dbapi.py Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland requested a review from pablogsal Jun 2, 2021
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

@vstinner vstinner commented Jun 2, 2021

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;
}

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 2, 2021

You can try _PyErr_ChainExceptions() to chain exceptions.

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

@pablogsal pablogsal merged commit 7ecd342 into python:main Aug 25, 2021
15 checks passed
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Aug 25, 2021

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

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 25, 2021

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

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 25, 2021

Thank you so much for reviewing, Pablo and Victor!

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 25, 2021

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

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 25, 2021

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

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 25, 2021

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

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 25, 2021

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

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 25, 2021
…ils to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 25, 2021

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

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 25, 2021
…ils to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 25, 2021
…ls to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 25, 2021
…ls to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 25, 2021

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

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 25, 2021
…ls to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 25, 2021
…ls to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 25, 2021
…ls to commit (pythonGH-26202)

Co-authored-by: Luca Citi
Co-authored-by: Berker Peksag <berker.peksag@gmail.com>.
(cherry picked from commit 7ecd342)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants