[PATCH] Support aborting the atomic context

[PATCH] Support aborting the atomic context

[Floris Bruynooghe]

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.

[PATCH] Support aborting the atomic context

[Floris Bruynooghe]

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

Re: [PATCH] Support aborting the atomic context

[David Bremner]

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

Re: [PATCH] Support aborting the atomic context

[David Bremner]

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

Re: [PATCH] Support aborting the atomic context

[Floris Bruynooghe]

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.