Kyle Stanley <aeros...@gmail.com> added the comment:
Upon further reading of the documentation and some experimentation, it would definitely not make sense to call `PyInterpreterState_Delete` here (since we're only closing the sub-interpreter in the current thread), so that's not the issue. I still have no idea as to why it's commented out there, but that's besides the point. I think I may have found the potential cause of the test failure though. Looking into `test_still_running()`, it's clear that the intention is for a RuntimeError to be raised if `interpreters.destroy()` is called on a currently running interpreter: ``` def test_still_running(self): main, = interpreters.list_all() interp = interpreters.create() with _running(interp): with self.assertRaises(RuntimeError): interpreters.destroy(interp) self.assertTrue(interpreters.is_running(interp)) ``` However, within interp_destroy (https://github.com/python/cpython/blob/56a45142e70a1ccf3233d43cb60c47255252e89a/Modules/_xxsubinterpretersmodule.c#L2024), it is apparent that the RuntimeError is ONLY raised when attempting to destroy the current interpreter: ``` if (interp == current) { PyErr_SetString(PyExc_RuntimeError, "cannot destroy the current interpreter"); return NULL; } ``` When attempting to destroy a running interpreter, NULL is returned without raising the RuntimeError: ``` if (_ensure_not_running(interp) < 0) { return NULL; } ``` This was my first guess at a solution: ``` if (_ensure_not_running(interp) < 0) { PyErr_SetString(PyExc_RuntimeError, "cannot destroy a running interpreter") return NULL; } ``` But, within `_ensure_not_running()` (https://github.com/python/cpython/blob/56a45142e70a1ccf3233d43cb60c47255252e89a/Modules/_xxsubinterpretersmodule.c#L1842), a RuntimeError is supposed to be raised from: ``` if (is_running) { PyErr_Format(PyExc_RuntimeError, "interpreter already running"); return -1; } ``` Initially, I was unsure of the difference between `PyErr_SetString()` and `PyErr_Format()`, so I referred to the documentation. `PyErr_Format()` is similar, but it also returns NULL. If I'm not mistaken doesn't mean that the `return -1` within the `if (is_running)` bock is effectively ignored? If so, this would potentially explain the problem... I think the appropriate solution would be to replace `PyErr_Format()` with `PyErr_SetString()` within `_ensure_not_running()`. Also, I think it would be more useful to additionally raise the RuntimeError within `interp_destroy()` if the interpreter is running, to provide a more useful and specific error message. "cannot destroy a running interpreter" is far more useful for debugging purposes than a more generic "interpreter already running". I plan on opening a PR to make these changes in the next few days. Let me know what you think Victor. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <https://bugs.python.org/issue37224> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com