Message352514
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. |
|
Date |
User |
Action |
Args |
2019-09-16 06:48:15 | aeros | set | recipients:
+ aeros, vstinner, eric.snow |
2019-09-16 06:48:15 | aeros | set | messageid: <1568616495.46.0.492892358093.issue37224@roundup.psfhosted.org> |
2019-09-16 06:48:15 | aeros | link | issue37224 messages |
2019-09-16 06:48:15 | aeros | create | |
|