Kyle Stanley <[email protected]> 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 <[email protected]>
<https://bugs.python.org/issue37224>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
