Add close/destroy methods; clean up descriptors

Previously, we would *never* call liberasurecode_instance_destroy for
any call to liberasurecode_instance_create. That was bad and would lead
to memory leaks -- while the capsule destructor would eventually free
the handle it wrapped, it would do nothing to clean up the associated
liberasurecode descriptor.

Now, use liberasurecode_instance_destroy to attempt to clean up the
descriptor. This can happen in the destructor (in which case any errors
are suppressed), or in a new `close` method on both ECPyECLibDriver and
ECDriver. If attempts are made to use a driver after close,
ECBackendInstanceNotAvailable will be raised, as the descriptor has
already been destroyed. The capsule destructor continues to be
responsible for freeing the handle.

Two new error modes are also handled in `pyeclib_c_seterr`, as they may
arise from liberasurecode_instance_destroy -- EDEADLK and EINVAL will
raise an ECDriverError, indicating trouble with the global instance lock.

Co-Authored-By: Tim Burke <tim.burke@gmail.com>
Closes-Bug: #1954352
Change-Id: I460014871a88fd7439efce3c5e8333c8447d1bdd

pyeclib/core.py

@@ -63,6 +63,11 @@ class ECPyECLibDriver(object):
             self.__class__.__name__, self.k, self.m, self.hd, self.ec_type,
             self.chksum_type)
 
+    def close(self):
+        if self.handle:
+            pyeclib_c.destroy(self.handle)
+        self.handle = None
+
     def encode(self, data_bytes):
         return pyeclib_c.encode(self.handle, data_bytes)
 
@@ -148,6 +153,9 @@ class ECNullDriver(object):
         self.m = m
         self.hd = hd
 
+    def close(self):
+        pass
+
     def encode(self, data_bytes):
         pass
 

pyeclib/ec_iface.py

@@ -250,6 +250,9 @@ class ECDriver(object):
             self.k,
             self.m)
 
+    def close(self):
+        self.ec_lib_reference.close()
+
     def encode(self, data_bytes):
         """
         Encode an arbitrary-sized string

src/c/pyeclib_c/pyeclib_c.c

@@ -177,6 +177,14 @@ void pyeclib_c_seterr(int ret, const char * prefix) {
             err_class = "ECOutOfMemory";
             err_msg = "Out of memory";
             break;
+        case EDEADLK:
+            err_class = "ECDriverError";
+            err_msg = "Thread already owns lock";
+            break;
+        case EINVAL:
+            err_class = "ECDriverError";
+            err_msg = "Invalid read-write lock";
+            break;
         default:
             err_class = "ECDriverError";
             err_msg = "Unknown error";
@@ -296,26 +304,65 @@ cleanup:
 }
 
 
+static pyeclib_t *
+_destroy(PyObject *obj, int in_destructor)
+{
+  pyeclib_t *pyeclib_handle = NULL;  /* pyeclib object to destroy */
+  int ret;
+
+  if (!PyCapsule_CheckExact(obj)) {
+    if (!in_destructor) {
+      pyeclib_c_seterr(-1, "pyeclib_c_destroy");
+    }
+    return NULL;
+  }
+
+  pyeclib_handle = (pyeclib_t*)PyCapsule_GetPointer(obj, PYECC_HANDLE_NAME);
+  if (pyeclib_handle == NULL) {
+    if (!in_destructor) {
+      pyeclib_c_seterr(-1, "pyeclib_c_destroy");
+    }
+    return NULL;
+  }
+
+  /*
+   * Free up the liberasure instance using liberasurecode.
+   * If it fails due to wrlock issues, fall back to check_and_free_buffer
+   * to GC the instance.
+   */
+  ret = liberasurecode_instance_destroy(pyeclib_handle->ec_desc);
+  if (ret != 0) {
+    if (in_destructor) {
+      /* destructor still wants to check_and_free */
+      return pyeclib_handle;
+    }
+    pyeclib_c_seterr(ret, "pyeclib_c_destroy");
+    return NULL;
+  }
+  return pyeclib_handle;
+}
+
+/**
+ * Destroy method for cleaning up pyeclib object.
+ */
+static PyObject *
+pyeclib_c_destroy(PyObject *self, PyObject *obj)
+{
+  if (_destroy(obj, 0) == NULL) {
+    return NULL;
+  }
+  Py_RETURN_NONE;
+}
+
+
 /**
  * Destructor method for cleaning up pyeclib object.
  */
 static void
 pyeclib_c_destructor(PyObject *obj)
 {
-  pyeclib_t *pyeclib_handle = NULL;  /* pyeclib object to destroy */
-
-  if (!PyCapsule_CheckExact(obj)) {
-    pyeclib_c_seterr(-1, "pyeclib_c_destructor");
-    return;
-  }
-
-  pyeclib_handle = (pyeclib_t*)PyCapsule_GetPointer(obj, PYECC_HANDLE_NAME);
-  if (pyeclib_handle == NULL) {
-    pyeclib_c_seterr(-1, "pyeclib_c_destructor");
-  } else {
-    check_and_free_buffer(pyeclib_handle);
-  }
-  return;
+  pyeclib_t *pyeclib_handle = _destroy(obj, 1);
+  check_and_free_buffer(pyeclib_handle);
 }
 
 
@@ -1189,6 +1236,7 @@ pyeclib_c_liberasurecode_version(PyObject *self, PyObject *args) {
 
 static PyMethodDef PyECLibMethods[] = {
     {"init",  pyeclib_c_init, METH_VARARGS, "Initialize a new erasure encoder/decoder"},
+    {"destroy",  pyeclib_c_destroy, METH_O, "Destroy an erasure encoder/decoder"},
     {"encode",  pyeclib_c_encode, METH_VARARGS, "Create parity using source data"},
     {"decode",  pyeclib_c_decode, METH_VARARGS, "Recover all lost data/parity"},
     {"reconstruct",  pyeclib_c_reconstruct, METH_VARARGS, "Recover selective data/parity"},

test/test_pyeclib_c.py

@@ -30,6 +30,7 @@ import unittest
 
 import pyeclib_c
 
+from pyeclib.ec_iface import ECBackendInstanceNotAvailable
 from pyeclib.ec_iface import PyECLib_EC_Types
 from pyeclib.ec_iface import VALID_EC_TYPES
 
@@ -469,6 +470,30 @@ class TestPyECLib(unittest.TestCase):
                     print("Reconstruct (%s): %s" %
                           (size_str, self.get_throughput(avg_time, size_str)))
 
+    def test_destroy(self):
+        # liberasurecode_rs_vand should always be available
+        handle = pyeclib_c.init(
+            4, 2, PyECLib_EC_Types.liberasurecode_rs_vand.value, 2)
+        whole_file_bytes = self.get_tmp_file("101-K").read()
+        # sanity check that it works
+        pyeclib_c.encode(handle, whole_file_bytes)
+        pyeclib_c.destroy(handle)
+
+        # Can't destroy again
+        with self.assertRaises(ECBackendInstanceNotAvailable) as caught:
+            pyeclib_c.destroy(handle)
+        self.assertEqual(
+            str(caught.exception),
+            'pyeclib_c_destroy ERROR: Backend instance not found. Please '
+            'inspect syslog for liberasurecode error report.')
+        with self.assertRaises(ECBackendInstanceNotAvailable) as caught:
+            # but now it's busted!
+            pyeclib_c.encode(handle, whole_file_bytes)
+        self.assertEqual(
+            str(caught.exception),
+            'pyeclib_c_encode ERROR: Backend instance not found. Please '
+            'inspect syslog for liberasurecode error report.')
+
 
 if __name__ == "__main__":
     unittest.main()