Author aeros
Recipients aeros, eric.snow, vstinner
Date 2019-09-16.06:48:15
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1568616495.46.0.492892358093.issue37224@roundup.psfhosted.org>
In-reply-to
Content
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.
History
Date User Action Args
2019-09-16 06:48:15aerossetrecipients: + aeros, vstinner, eric.snow
2019-09-16 06:48:15aerossetmessageid: <1568616495.46.0.492892358093.issue37224@roundup.psfhosted.org>
2019-09-16 06:48:15aeroslinkissue37224 messages
2019-09-16 06:48:15aeroscreate