Cown release (#44)

* Cown release

A cown will now be released:
- When it is in pending-release state and its containing region closes
- When a closed region, other cown or immutable object it stored in it
- At the end of a @using if the cown's contained region can be closed

* TODO => TODO Pyrona:

Co-authored-by: Fridtjof Stoldt <xFrednet@gmail.com>

* Remove GIL operations in release (since this operation cannot be contended)

---------

Co-authored-by: Fridtjof Stoldt <xFrednet@gmail.com>

Author: TobiasWrigstad

Include/internal/pycore_regions.h

@@ -85,9 +85,13 @@ int _Py_CheckRegionInvariant(PyThreadState *tstate);
 // Set a cown as parent of a region
 void _PyRegion_set_cown_parent(PyObject* region, PyObject* cown);
 // Check whether a region is closed
-int _PyRegion_is_closed(PyObject* region);
 int _PyCown_release(PyObject *self);
 int _PyCown_is_released(PyObject *self);
+int _PyCown_is_pending_release(PyObject *self);
+PyObject *_PyCown_close_region(PyObject* ob);
+#define PyCown_close_region(op) _PyCown_close_region(_PyObject_CAST(op))
+int _PyRegion_is_closed(PyObject* op);
+#define PyRegion_is_closed(op) _PyRegion_is_closed(_PyObject_CAST(op))
 
 
 #ifdef _Py_TYPEOF

Lib/test/test_using.py

@@ -96,7 +96,7 @@ def _():
             self.assertTrue(self.hacky_state_check(c, "acquired"))
             r = None
             c.get().close()
-            self.assertTrue(self.hacky_state_check(c, "released"))
+            self.assertTrue(self.hacky_state_check(c, "acquired"))
         self.assertTrue(self.hacky_state_check(c, "released"))
 
     def test_region_cown_ptr(self):

Objects/cown.c

@@ -19,6 +19,10 @@
 #include "pyerrors.h"
 #include "pystate.h"
 
+// Needed to test for region object
+extern PyTypeObject PyRegion_Type;
+extern PyTypeObject PyCown_Type;
+
 typedef enum {
     Cown_RELEASED        = 0,
     Cown_ACQUIRED        = 1,
@@ -57,7 +61,7 @@ static void PyCown_dealloc(PyCownObject *self) {
 }
 
 static int PyCown_init(PyCownObject *self, PyObject *args, PyObject *kwds) {
-    // TODO: should not be needed in the future
+    // TODO: Pyrona: should not be needed in the future
     _Py_MakeImmutable(_PyObject_CAST(Py_TYPE(self)));
     _Py_notify_regions_in_use();
 
@@ -128,10 +132,12 @@ static int PyCown_traverse(PyCownObject *self, visitproc visit, void *arg) {
 // compatible with PyCFunction
 static PyObject *PyCown_acquire(PyCownObject *self, PyObject *ignored) {
     PyThreadState *tstate = PyThreadState_Get();
+
+    // TODO: Pyrona: releasing the GIL will eventually not be necessary here
     Py_BEGIN_ALLOW_THREADS
     int expected = Cown_RELEASED;
 
-    // TODO: eventually replace this with something from pycore_atomic (nothing there now)
+    // TODO: Pyrona: eventually replace this with something from pycore_atomic (nothing there now)
     while (!atomic_compare_exchange_strong(&self->state._value, &expected, Cown_ACQUIRED)) {
         expected = Cown_RELEASED;
         sem_wait(&self->semaphore);
@@ -155,11 +161,16 @@ static PyObject *PyCown_release(PyCownObject *self, PyObject *ignored) {
 
     BAIL_UNLESS_OWNED(self, "Thread attempted to release a cown it did not own");
 
-    Py_BEGIN_ALLOW_THREADS
+    if (self->value && Py_TYPE(self->value) == &PyRegion_Type) {
+        if (PyCown_close_region(self->value) == NULL) {
+            // Close region failed -- propagate its error
+            return NULL;
+        }
+    }
+
     self->owning_thread = 0;
     _Py_atomic_store(&self->state, Cown_RELEASED);
     sem_post(&self->semaphore);
-    Py_END_ALLOW_THREADS
 
     Py_RETURN_NONE;
 }
@@ -174,6 +185,13 @@ int _PyCown_is_released(PyObject *self) {
     return STATE(cown) == Cown_RELEASED;
 }
 
+int _PyCown_is_pending_release(PyObject *self) {
+    assert(Py_TYPE(self) == &PyCown_Type && "Is pending release called on non-cown!");
+
+    PyCownObject *cown = _Py_CAST(PyCownObject *, self);
+    return STATE(cown) == Cown_PENDING_RELEASE;
+}
+
 // The ignored argument is required for this function's type to be
 // compatible with PyCFunction
 static PyObject *PyCown_get(PyCownObject *self, PyObject *ignored) {
@@ -186,16 +204,12 @@ static PyObject *PyCown_get(PyCownObject *self, PyObject *ignored) {
     }
 }
 
-// Needed to test for region object
-extern PyTypeObject PyRegion_Type;
-extern PyTypeObject PyCown_Type;
-
 static PyObject *PyCown_set_unchecked(PyCownObject *self, PyObject *arg) {
     // Cowns are cells that hold a reference to a bridge object,
     // (or another cown or immutable object)
-    const bool is_region_object =
+    const bool arg_is_region_object =
         Py_IS_TYPE(arg, &PyRegion_Type) && _Py_is_bridge_object(arg);
-    if (is_region_object ||
+    if (arg_is_region_object ||
         arg->ob_type == &PyCown_Type ||
         _Py_IsImmutable(arg)) {
 
@@ -205,10 +219,14 @@ static PyObject *PyCown_set_unchecked(PyCownObject *self, PyObject *arg) {
 
         // Tell the region that it is owned by a cown,
         // to enable it to release the cown on close
-        if (is_region_object) {
+        if (arg_is_region_object) {
             _PyRegion_set_cown_parent(arg, _PyObject_CAST(self));
+            // TODO: Pyrona: should not run try close here unless dirty at the end of phase 3
+            // if (_PyCown_close_region(arg) == Py_None) {
             if (_PyRegion_is_closed(arg)) {
-                PyCown_release(self, NULL);
+                if (PyCown_release(self, NULL) == NULL) {
+                    PyErr_Clear();
+                }
             } else {
                 _Py_atomic_store(&self->state, Cown_PENDING_RELEASE);
                 PyThreadState *tstate = PyThreadState_Get();

Objects/regions.c

@@ -219,8 +219,8 @@ static int regionmetadata_close(regionmetadata* self) {
         return regionmetadata_dec_osc(parent);
     }
 
-    // Check if in a cown -- if so, release cown
-    if (self->cown) {
+    // Check if in a cown which is waiting for the region to close -- if so, release cown
+    if (self->cown && _PyCown_is_pending_release(self->cown)) {
         // Propagate error from release
         return _PyCown_release(self->cown);
     }
@@ -1749,13 +1749,16 @@ int _PyRegion_is_closed(PyObject* self) {
 // The ignored argument is required for this function's type to be
 // compatible with PyCFunction
 static PyObject *PyRegion_close(PyRegionObject *self, PyObject *ignored) {
-    regionmetadata* const md = REGION_DATA_CAST(Py_REGION(self));
-    if (!regionmetadata_is_open(md)) {
+    if (PyRegion_is_closed(self)) {
         Py_RETURN_NONE; // Double close is OK
     }
 
     // Attempt to close the region
     if (try_close(self) != 0) {
+        if (!PyErr_Occurred()) {
+            // try_close did not run out of memory but failed to close the region
+            PyErr_Format(PyExc_RegionError, "Attempting to close the region failed");
+        }
         return NULL;
     }
 
@@ -2009,3 +2012,13 @@ void _Py_RegionRemoveReference(PyObject *src, PyObject *tgt) {
     // Unparent the region.
     regionmetadata_set_parent(tgt_md, NULL);
 }
+
+PyObject *_PyCown_close_region(PyObject* ob) {
+    if (Py_TYPE(ob) == &PyRegion_Type) {
+        // Attempt to close the region
+        return PyRegion_close(_Py_CAST(PyRegionObject*, ob), NULL);
+    } else {
+        PyErr_SetString(PyExc_RegionError, "Attempted to close a region through a non-bridge object");
+        return NULL;
+    }
+}