)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"bfc7ab7a217b901d19bac6eed49cf369611b9440","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"adcdcdff_620e14fb","updated":"2024-09-23 02:00:12.000000000","message":"-1 mainly for the error-swallowing. I wonder though if it\u0027d be better to add a `close` method and have the destructor call *that* -- I feel like I rarely write `del thing`, but reasonably often write `thing.close()`. Plus, I\u0027m not sure that `del` has any promises about the *timing* of destructor calls...","commit_id":"a5baddd47131b6bf908c4adaac6e7b6518c8d4e4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"22219a2501ebe8946fba37a449eb73a25e2d5da2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0b488678_fef2a753","updated":"2024-09-23 01:48:38.000000000","message":"Was playing with a test. And went down a bunch of rabbitholes to test it nicely.\n\nPushing this version up because it\u0027s the simplest.\n\nI did try and rip the destroy_instance into another python method so I could mock the return code.. but then the destructor needs to be done in python.\n\nStill haven\u0027t figured a good way to mock out a c call in python from a python distrcutor that is actaully in c 😞\n\nSo atm no test 😞","commit_id":"a5baddd47131b6bf908c4adaac6e7b6518c8d4e4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"4377f339fd340f4a4d0f2383aa2ac666f03fa94d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"4f5b0a60_45b4c39c","updated":"2024-09-23 04:02:50.000000000","message":"ahh actually they\u0027re libc errnos. So maybe they\u0027ll just work. Let me rework this","commit_id":"a5baddd47131b6bf908c4adaac6e7b6518c8d4e4"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"d029345a16bed036300a2118760f61831a13ed42","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"9a872105_018d2b91","updated":"2024-10-10 19:01:28.000000000","message":"OK, so https://review.opendev.org/c/openstack/pyeclib/+/931832 is getting close -- do we want to squash it into this? Or keep them separate? After digging into it more, I\u0027m not sure we really want to be calling `pyeclib_c_seterr` in the destructor at all...","commit_id":"522f7f28565680936bcae161fe24f9f6b3855e9a"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"adef79f86c95f4c0dcfe352033bfc018fd466a48","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"c9d38590_8ef705d7","updated":"2024-10-08 22:55:53.000000000","message":"recheck\n\nhttps://review.opendev.org/c/openstack/pyeclib/+/931846 fixed the py35 job.","commit_id":"522f7f28565680936bcae161fe24f9f6b3855e9a"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"09088e96f7defdeb3316c7e5fd0ab85d3ecdb358","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"481a44a8_5ac5007a","in_reply_to":"9a872105_018d2b91","updated":"2024-10-10 23:21:18.000000000","message":"Either way is fine. Fell free to squash and co-author yourself. I do think 931832 is definitely the direction we want to continue to move in!","commit_id":"522f7f28565680936bcae161fe24f9f6b3855e9a"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"7c61c6dfc91a1183f5656e35eb6563c2781f3b7c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"bbf6ce7f_910579d9","updated":"2024-10-16 04:41:50.000000000","message":"Nice work Tim, let\u0027s get this in!\n\nEven if I\u0027m the author.. I think we\u0027ve done a pretty good jobs of testing this.","commit_id":"30a8f257808fa0e284cf7d6cd3fcc3468cfb093c"}],"src/c/pyeclib_c/pyeclib_c.c":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"bfc7ab7a217b901d19bac6eed49cf369611b9440","unresolved":true,"context_lines":[{"line_number":329,"context_line":"      case -EBACKENDNOTAVAIL:"},{"line_number":330,"context_line":"        pyeclib_c_seterr(-EBACKENDNOTAVAIL, \"pyeclib_c_destructor\");"},{"line_number":331,"context_line":"        break;"},{"line_number":332,"context_line":"      default:"},{"line_number":333,"context_line":"        /* Just fall through to check_and_free_buffer */"},{"line_number":334,"context_line":"    }"},{"line_number":335,"context_line":"    check_and_free_buffer(pyeclib_handle);"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"224b3f0a_76dddcc6","line":332,"updated":"2024-09-23 02:00:12.000000000","message":"OK, my read on it is that the other possible non-zero returns would be `EINVAL` or `EDEADLK` if there\u0027s any trouble with the lock (based on https://linux.die.net/man/3/pthread_rwlock_wrlock). Neither *should* happen, but I think I\u0027d still prefer that we do something like\n```\nif (ret !\u003d 0) {\n    pyeclib_c_seterr(ret, \"pyeclib_c_destructor\");\n}\n```\nSwallowing the error entirely seems likely to cause confusion later (if we ever trip it). This also leaves us more resilient in case liberasurecode needs to start returning other non-lock-related errors.","commit_id":"a5baddd47131b6bf908c4adaac6e7b6518c8d4e4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"069a755c78439e86411d9d29533ed0886b852243","unresolved":true,"context_lines":[{"line_number":329,"context_line":"      case -EBACKENDNOTAVAIL:"},{"line_number":330,"context_line":"        pyeclib_c_seterr(-EBACKENDNOTAVAIL, \"pyeclib_c_destructor\");"},{"line_number":331,"context_line":"        break;"},{"line_number":332,"context_line":"      default:"},{"line_number":333,"context_line":"        /* Just fall through to check_and_free_buffer */"},{"line_number":334,"context_line":"    }"},{"line_number":335,"context_line":"    check_and_free_buffer(pyeclib_handle);"}],"source_content_type":"text/x-csrc","patch_set":1,"id":"2d5b689b_0b3cfdbd","line":332,"in_reply_to":"224b3f0a_76dddcc6","updated":"2024-09-23 02:21:25.000000000","message":"yeah, good point. I initially had a \n```ifdef_errocode_h````\n\nTo catch and deal with them properly. but yeah, your way is better because they\u0027re no swollowed up. Good call.","commit_id":"a5baddd47131b6bf908c4adaac6e7b6518c8d4e4"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"584ef65a50e13942742a84e0a7fdf158d02e6af3","unresolved":true,"context_lines":[{"line_number":182,"context_line":"            err_class \u003d \"ECOutOfMemory\";"},{"line_number":183,"context_line":"            err_msg \u003d \"Out of memory\";"},{"line_number":184,"context_line":"            break;"},{"line_number":185,"context_line":"        case -EDEADLK:"},{"line_number":186,"context_line":"            err_class \u003d \"ECDriverError\";"},{"line_number":187,"context_line":"            err_msg \u003d \"Thread already owns lock\";"},{"line_number":188,"context_line":"            break;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"5156616d_30d2b705","line":185,"range":{"start_line":185,"start_character":13,"end_line":185,"end_character":21},"updated":"2024-09-23 18:03:51.000000000","message":"I don\u0027t think these will come out of `pthread_rwlock_wrlock` negative. Though I couldn\u0027t get *anything* to pop with\n```\ndiff --git a/test/test_pyeclib_api.py b/test/test_pyeclib_api.py\nindex 3268582..c1d15bd 100644\n--- a/test/test_pyeclib_api.py\n+++ b/test/test_pyeclib_api.py\n@@ -21,6 +21,8 @@\n # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF\n # THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n \n+import ctypes\n+import ctypes.util\n import os\n import random\n import resource\n@@ -180,6 +182,20 @@ class TestPyECLibDriver(unittest.TestCase):\n                 pass\n         self.assertEqual(available_ec_types, VALID_EC_TYPES)\n \n+    def test_destroy_while_lock_held(self):\n+        ec \u003d ECDriver(k\u003d10, m\u003d5, ec_type\u003d\u0027liberasurecode_rs_vand\u0027)\n+        libc \u003d ctypes.CDLL(ctypes.util.find_library(\u0027c\u0027))\n+        libec \u003d ctypes.CDLL(ctypes.util.find_library(\u0027erasurecode\u0027))\n+        self.assertEqual(0, libc.pthread_rwlock_wrlock(\n+            libec.active_instances_rwlock))\n+        try:\n+            del ec\n+            import gc; gc.collect()\n+        finally:\n+            self.assertEqual(0, libc.pthread_rwlock_unlock(\n+                libec.active_instances_rwlock))\n+\n     def test_valid_algo(self):\n         print(\"\")\n         for _type in ALL_EC_TYPES:\n```\nNot even a warning about `Exception ignored in: ...` when I threw a `seld.fail()` at the end. Digging around to try to destroy the underlying handle at `ec.ec_lib_reference.handle` didn\u0027t help, either.\n\nThough even if I could make that test work, I don\u0027t actually **want** to keep `active_instances_rwlock` as a public API in liberasurecode.","commit_id":"92c67d21404181fb6798abf8a0efab51d8dd9637"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"31aff71424590d349ef83f66aac44558f547241c","unresolved":true,"context_lines":[{"line_number":182,"context_line":"            err_class \u003d \"ECOutOfMemory\";"},{"line_number":183,"context_line":"            err_msg \u003d \"Out of memory\";"},{"line_number":184,"context_line":"            break;"},{"line_number":185,"context_line":"        case -EDEADLK:"},{"line_number":186,"context_line":"            err_class \u003d \"ECDriverError\";"},{"line_number":187,"context_line":"            err_msg \u003d \"Thread already owns lock\";"},{"line_number":188,"context_line":"            break;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"aea03784_85b5dc7f","line":185,"range":{"start_line":185,"start_character":13,"end_line":185,"end_character":21},"in_reply_to":"0bb93e8c_31e53543","updated":"2024-09-25 06:50:57.000000000","message":"If I just add another:\n\n```\n+        # Attempting to take another lock does fail as expected\n+        self.assertEqual(errno.EDEADLK, libc.pthread_rwlock_wrlock(\n+            libec.active_instances_rwlock))\n```\n\nInto your test we get back the EDEADLK. So I guess we have a lock but in a different process, so not actually grabbing the threadlock the python object has.","commit_id":"92c67d21404181fb6798abf8a0efab51d8dd9637"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"3dbcbbf73948e2cffc134580174c888fb3047545","unresolved":true,"context_lines":[{"line_number":182,"context_line":"            err_class \u003d \"ECOutOfMemory\";"},{"line_number":183,"context_line":"            err_msg \u003d \"Out of memory\";"},{"line_number":184,"context_line":"            break;"},{"line_number":185,"context_line":"        case -EDEADLK:"},{"line_number":186,"context_line":"            err_class \u003d \"ECDriverError\";"},{"line_number":187,"context_line":"            err_msg \u003d \"Thread already owns lock\";"},{"line_number":188,"context_line":"            break;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"0bb93e8c_31e53543","line":185,"range":{"start_line":185,"start_character":13,"end_line":185,"end_character":21},"in_reply_to":"5156616d_30d2b705","updated":"2024-09-24 01:10:24.000000000","message":"We could possibly try and get a rwlock on a different int so it\u0027ll call an EINVAL. But yeah, I should remove the -. that\u0027s true, they should come out as a positive int. We just pass them as -\u0027ves.","commit_id":"92c67d21404181fb6798abf8a0efab51d8dd9637"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"a8b658cf714d02011d051f7562f8a560fec17167","unresolved":true,"context_lines":[{"line_number":182,"context_line":"            err_class \u003d \"ECOutOfMemory\";"},{"line_number":183,"context_line":"            err_msg \u003d \"Out of memory\";"},{"line_number":184,"context_line":"            break;"},{"line_number":185,"context_line":"        case -EDEADLK:"},{"line_number":186,"context_line":"            err_class \u003d \"ECDriverError\";"},{"line_number":187,"context_line":"            err_msg \u003d \"Thread already owns lock\";"},{"line_number":188,"context_line":"            break;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"ac7eca2d_e465c393","line":185,"range":{"start_line":185,"start_character":13,"end_line":185,"end_character":21},"in_reply_to":"53b1dd33_fb7e6435","updated":"2024-10-08 18:01:51.000000000","message":"Maybe something like https://review.opendev.org/c/openstack/pyeclib/+/931832 ?","commit_id":"92c67d21404181fb6798abf8a0efab51d8dd9637"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"9d36436bba3c2cada94d438f6e689ec1b29234a9","unresolved":true,"context_lines":[{"line_number":182,"context_line":"            err_class \u003d \"ECOutOfMemory\";"},{"line_number":183,"context_line":"            err_msg \u003d \"Out of memory\";"},{"line_number":184,"context_line":"            break;"},{"line_number":185,"context_line":"        case -EDEADLK:"},{"line_number":186,"context_line":"            err_class \u003d \"ECDriverError\";"},{"line_number":187,"context_line":"            err_msg \u003d \"Thread already owns lock\";"},{"line_number":188,"context_line":"            break;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"53b1dd33_fb7e6435","line":185,"range":{"start_line":185,"start_character":13,"end_line":185,"end_character":21},"in_reply_to":"aea03784_85b5dc7f","updated":"2024-10-08 04:51:07.000000000","message":"Some printf-debugging got me to a point that I could trigger the new message: https://paste.opendev.org/show/brwVrGYAuv5hKkhhN6BL/\n\nBut it\u0027s mostly left me scratching my head more than ever. It does **not** blow up where I expect (and as a result, blows up every following test, as the lock is not released), and if I uncomment the manual garbage-collection, *the error is never raised* even though I see all the messages from the destructor running??\n\nI can\u0027t help but wonder if it might be better for us to add a `deinit` to pyeclib_c and then hang a `close` off `ECPyECLibDriver` or something, and avoid messing with destructors if at all possible.","commit_id":"92c67d21404181fb6798abf8a0efab51d8dd9637"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"9d36436bba3c2cada94d438f6e689ec1b29234a9","unresolved":true,"context_lines":[{"line_number":294,"context_line":"    pyeclib_c_seterr(-EINVALIDPARAMS, \"pyeclib_c_init\");"},{"line_number":295,"context_line":"    goto cleanup;"},{"line_number":296,"context_line":"  } else {"},{"line_number":297,"context_line":"    Py_INCREF(pyeclib_obj_handle);"},{"line_number":298,"context_line":"  }"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"exit:"}],"source_content_type":"text/x-csrc","patch_set":3,"id":"e909ab85_8f17c847","line":297,"updated":"2024-10-08 04:51:07.000000000","message":"Oh, fun -- I think this is a leak: the `PyCapsule_New` should already be giving us a strong reference.","commit_id":"97364018d0c635e08cfa1f428476195da0ecc13c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"a8b658cf714d02011d051f7562f8a560fec17167","unresolved":false,"context_lines":[{"line_number":294,"context_line":"    pyeclib_c_seterr(-EINVALIDPARAMS, \"pyeclib_c_init\");"},{"line_number":295,"context_line":"    goto cleanup;"},{"line_number":296,"context_line":"  } else {"},{"line_number":297,"context_line":"    Py_INCREF(pyeclib_obj_handle);"},{"line_number":298,"context_line":"  }"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"exit:"}],"source_content_type":"text/x-csrc","patch_set":3,"id":"e995eac7_092d1878","line":297,"in_reply_to":"e909ab85_8f17c847","updated":"2024-10-08 18:01:51.000000000","message":"https://review.opendev.org/c/openstack/pyeclib/+/931822\n\nAlso, vaguely related: https://review.opendev.org/c/openstack/pyeclib/+/931825","commit_id":"97364018d0c635e08cfa1f428476195da0ecc13c"}],"test/test_pyeclib_c.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4920f61f9e363606bc5da8801377c9ea121de59f","unresolved":true,"context_lines":[{"line_number":481,"context_line":""},{"line_number":482,"context_line":"        # Can\u0027t destroy again"},{"line_number":483,"context_line":"        with self.assertRaises(ECBackendInstanceNotAvailable) as caught:"},{"line_number":484,"context_line":"            pyeclib_c.destroy(handle)"},{"line_number":485,"context_line":"        self.assertEqual("},{"line_number":486,"context_line":"            str(caught.exception),"},{"line_number":487,"context_line":"            \u0027pyeclib_c_destroy ERROR: Backend instance not found. Please \u0027"}],"source_content_type":"text/x-python","patch_set":5,"id":"d7ca821c_09ca4317","line":484,"updated":"2024-10-11 14:39:44.000000000","message":"Brought up in the other patch: Arguably, we could just swallow this exception. Caller wanted it destroyed, it\u0027s destroyed. Of course, we\u0027d still want to watch for *other* error codes.","commit_id":"30a8f257808fa0e284cf7d6cd3fcc3468cfb093c"}]}