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

[erlend-aasland]

https://bugs.python.org/issue27334

[erlend-aasland]

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

(cc. @pablogsal)

[lciti]

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]

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.

[vstinner]

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]

You can try _PyErr_ChainExceptions() to chain exceptions.

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

[pablogsal]

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

[erlend-aasland]

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

[erlend-aasland]

Thank you so much for reviewing, Pablo and Victor!

[miss-islington]

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

[miss-islington]

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

[miss-islington]

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]

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

[bedevere-bot]

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

[bedevere-bot]

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