* [PATCH] Support aborting the atomic context @ 2020-06-14 15:23 Floris Bruynooghe 2020-06-14 15:23 ` Floris Bruynooghe 2020-06-15 10:35 ` David Bremner 0 siblings, 2 replies; 5+ messages in thread From: Floris Bruynooghe @ 2020-06-14 15:23 UTC (permalink / raw) To: notmuch This is an implementation of what was suggested in id:87v9k96xtl.fsf@powell.devork.be It closes the database as that is the only safe way to do this afaik. Currently when the database is closed there are still a bunch of operations which can result in segfaults. Yet the API also promises that some operations are still valid, basically those which only access already previously retrieved information. It would be nice to find a good solution for this in the python bindings, but I don't yet know what this would be. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Support aborting the atomic context 2020-06-14 15:23 [PATCH] Support aborting the atomic context Floris Bruynooghe @ 2020-06-14 15:23 ` Floris Bruynooghe 2020-06-16 11:22 ` David Bremner 2020-06-15 10:35 ` David Bremner 1 sibling, 1 reply; 5+ messages in thread From: Floris Bruynooghe @ 2020-06-14 15:23 UTC (permalink / raw) To: notmuch Since it is possible to use an atomic context to abort a number of changes support this usage. Because the only way to actually abort the transaction is to close the database this must also do so. --- bindings/python-cffi/notmuch2/_database.py | 16 +++++++++++++++- bindings/python-cffi/tests/test_database.py | 5 +++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py index 95f59ca0..c851f0a5 100644 --- a/bindings/python-cffi/notmuch2/_database.py +++ b/bindings/python-cffi/notmuch2/_database.py @@ -641,6 +641,7 @@ class AtomicContext: def __init__(self, db, ptr_name): self._db = db self._ptr = lambda: getattr(db, ptr_name) + self._exit_fn = lambda: None def __del__(self): self._destroy() @@ -656,13 +657,17 @@ class AtomicContext: ret = capi.lib.notmuch_database_begin_atomic(self._ptr()) if ret != capi.lib.NOTMUCH_STATUS_SUCCESS: raise errors.NotmuchError(ret) + self._exit_fn = self._end_atomic return self - def __exit__(self, exc_type, exc_value, traceback): + def _end_atomic(self): ret = capi.lib.notmuch_database_end_atomic(self._ptr()) if ret != capi.lib.NOTMUCH_STATUS_SUCCESS: raise errors.NotmuchError(ret) + def __exit__(self, exc_type, exc_value, traceback): + self._exit_fn() + def force_end(self): """Force ending the atomic section. @@ -681,6 +686,15 @@ class AtomicContext: if ret != capi.lib.NOTMUCH_STATUS_SUCCESS: raise errors.NotmuchError(ret) + def abort(self): + """Abort the transaction. + + Aborting a transaction will not commit any of the changes, but + will also implicitly close the database. + """ + self._exit_fn = lambda: None + self._db.close() + @functools.total_ordering class DbRevision: diff --git a/bindings/python-cffi/tests/test_database.py b/bindings/python-cffi/tests/test_database.py index e3a8344d..aa2cbdc7 100644 --- a/bindings/python-cffi/tests/test_database.py +++ b/bindings/python-cffi/tests/test_database.py @@ -127,6 +127,11 @@ class TestAtomic: with pytest.raises(errors.UnbalancedAtomicError): ctx.force_end() + def test_abort(self, db): + with db.atomic() as txn: + txn.abort() + assert db.closed + class TestRevision: -- 2.27.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Support aborting the atomic context 2020-06-14 15:23 ` Floris Bruynooghe @ 2020-06-16 11:22 ` David Bremner 0 siblings, 0 replies; 5+ messages in thread From: David Bremner @ 2020-06-16 11:22 UTC (permalink / raw) To: Floris Bruynooghe, notmuch Floris Bruynooghe <flub@devork.be> writes: > Since it is possible to use an atomic context to abort a number of > changes support this usage. Because the only way to actually abort > the transaction is to close the database this must also do so. merged to release and master, with a note about the anticipated update path. d ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Support aborting the atomic context 2020-06-14 15:23 [PATCH] Support aborting the atomic context Floris Bruynooghe 2020-06-14 15:23 ` Floris Bruynooghe @ 2020-06-15 10:35 ` David Bremner 2020-06-15 19:55 ` Floris Bruynooghe 1 sibling, 1 reply; 5+ messages in thread From: David Bremner @ 2020-06-15 10:35 UTC (permalink / raw) To: Floris Bruynooghe, notmuch Floris Bruynooghe <flub@devork.be> writes: > This is an implementation of what was suggested in > id:87v9k96xtl.fsf@powell.devork.be It closes the database as that is > the only safe way to do this afaik. > > Currently when the database is closed there are still a bunch of > operations which can result in segfaults. Yet the API also promises > that some operations are still valid, basically those which only > access already previously retrieved information. It would be nice to > find a good solution for this in the python bindings, but I don't yet > know what this would be. There is a Xapian method WritableDatabase::cancel_transaction(), but it is not currently exposed by notmuch. I think it could be added, but getting the testing working right is not something I want to tackle at this point in the release cycle. I guess this should be upwardly compatible, as all code correct for your current implementation should still work if we replace notmuch_database_close with notmuch_cancel_atomic. Thoughts? d ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Support aborting the atomic context 2020-06-15 10:35 ` David Bremner @ 2020-06-15 19:55 ` Floris Bruynooghe 0 siblings, 0 replies; 5+ messages in thread From: Floris Bruynooghe @ 2020-06-15 19:55 UTC (permalink / raw) To: David Bremner, notmuch On Mon 15 Jun 2020 at 07:35 -0300, David Bremner wrote: > Floris Bruynooghe <flub@devork.be> writes: > >> This is an implementation of what was suggested in >> id:87v9k96xtl.fsf@powell.devork.be It closes the database as that is >> the only safe way to do this afaik. >> >> Currently when the database is closed there are still a bunch of >> operations which can result in segfaults. I realise that this is perhaps not a very helpful comment. I'll see if I can narrow this down into a proper bug report. >> Yet the API also promises >> that some operations are still valid, basically those which only >> access already previously retrieved information. It would be nice to >> find a good solution for this in the python bindings, but I don't yet >> know what this would be. > > There is a Xapian method WritableDatabase::cancel_transaction(), but it > is not currently exposed by notmuch. I think it could be added, but > getting the testing working right is not something I want to tackle at > this point in the release cycle. I guess this should be upwardly > compatible, as all code correct for your current implementation should > still work if we replace notmuch_database_close with > notmuch_cancel_atomic. Thoughts? I agree with your reasoning here that such a change would be upwardly compatible. Though I can think of some really convoluted scenarios where this could be false, e.g. after aborting a transaction code could rely on handling NOTMUCH_STATUS_XAPIAN_EXCEPTION it gets from a the closed database. This seems so much a corner case though, that I'd be willing to ignore it. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-16 11:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-14 15:23 [PATCH] Support aborting the atomic context Floris Bruynooghe 2020-06-14 15:23 ` Floris Bruynooghe 2020-06-16 11:22 ` David Bremner 2020-06-15 10:35 ` David Bremner 2020-06-15 19:55 ` Floris Bruynooghe
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).