unofficial mirror of notmuch@notmuchmail.org
 help / color / Atom feed
* [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	[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

* 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

end of thread, back to index

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

unofficial mirror of notmuch@notmuchmail.org

Archives are clonable:
	git clone --mirror https://yhetil.org/notmuch/0 notmuch/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 notmuch notmuch/ https://yhetil.org/notmuch \
		notmuch@notmuchmail.org
	public-inbox-index notmuch

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.mail.notmuch.general
	nntp://news.gmane.io/gmane.mail.notmuch.general


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git