Skip to content

bpo-30696: Fix the REPL looping endlessly when no memory #4160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 12, 2017
Merged

bpo-30696: Fix the REPL looping endlessly when no memory #4160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
560ccee
bpo-30696: Fix the REPL looping endlessly when no memory
xdegaye Oct 28, 2017
15230d1
Do not break backward compatibility
xdegaye Oct 29, 2017
ac6003a
Call _PyDebug_PrintTotalRefs() after printing the exception
xdegaye Oct 29, 2017
44c335e
Create test_repl.py and move run_pty() to test.support
xdegaye Oct 29, 2017
be1df91
Update Misc/NEWS.
xdegaye Oct 29, 2017
477f9c5
Do not call Py_FatalError() and return an error
xdegaye Nov 5, 2017
1516d4c
Merge remote-tracking branch 'origin/master' into bpo-30696
xdegaye Nov 5, 2017
1fe98b2
Updates after the second round of review
xdegaye Nov 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Doc/c-api/veryhigh.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ the same library that the Python runtime is using.
Read and execute statements from a file associated with an interactive device
until EOF is reached. The user will be prompted using ``sys.ps1`` and
``sys.ps2``. *filename* is decoded from the filesystem encoding
(:func:`sys.getfilesystemencoding`). Returns ``0`` at EOF.
(:func:`sys.getfilesystemencoding`). Returns ``0`` at EOF or a negative
number upon failure.


.. c:var:: int (*PyOS_InputHook)(void)
Expand Down
62 changes: 62 additions & 0 deletions Lib/test/test_repl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
"""Test the interactive interpreter."""

import sys
import os
import unittest
import subprocess
from textwrap import dedent
from test.support import cpython_only, SuppressCrashReport
from test.support.script_helper import kill_python

def spawn_repl(*args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kw):
"""Run the Python REPL with the given arguments.

kw is extra keyword args to pass to subprocess.Popen. Returns a Popen
object.
"""

# To run the REPL without using a terminal, spawn python with the command
# line option '-i' and the process name set to '<stdin>'.
# The directory of argv[0] must match the directory of the Python
# executable for the Popen() call to python to succeed as the directory
# path may be used by Py_GetPath() to build the default module search
# path.
stdin_fname = os.path.join(os.path.dirname(sys.executable), "<stdin>")
cmd_line = [stdin_fname, '-E', '-i']
cmd_line.extend(args)

# Set TERM=vt100, for the rationale see the comments in spawn_python() of
# test.support.script_helper.
env = kw.setdefault('env', dict(os.environ))
env['TERM'] = 'vt100'
return subprocess.Popen(cmd_line, executable=sys.executable,
stdin=subprocess.PIPE,
stdout=stdout, stderr=stderr,
**kw)

class TestInteractiveInterpreter(unittest.TestCase):

@cpython_only
def test_no_memory(self):
# Issue #30696: Fix the interactive interpreter looping endlessly when
# no memory. Check also that the fix does not break the interactive
# loop when an exception is raised.
user_input = """
import sys, _testcapi
1/0
print('After the exception.')
_testcapi.set_nomemory(0)
sys.exit(0)
"""
user_input = dedent(user_input)
user_input = user_input.encode()
p = spawn_repl()
with SuppressCrashReport():
p.stdin.write(user_input)
output = kill_python(p)
self.assertIn(b'After the exception.', output)
# Exit code 120: Py_FinalizeEx() failed to flush stdout and stderr.
self.assertIn(p.returncode, (1, 120))

if __name__ == "__main__":
unittest.main()
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core and Builtins/2017-10-28-22-06-03.bpo-30696.lhC3HE.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix the interactive interpreter looping endlessly when no memory.
60 changes: 42 additions & 18 deletions Python/pythonrun.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ static PyObject *run_pyc_file(FILE *, const char *, PyObject *, PyObject *,
PyCompilerFlags *);
static void err_input(perrdetail *);
static void err_free(perrdetail *);
static int PyRun_InteractiveOneObjectEx(FILE *, PyObject *, PyCompilerFlags *);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that we need to make this new function public. I propose to make it private.

Please add also a short comment to explain that it's similar to PyRun_InteractiveOneObject() but don't print the error on failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand your comment, this new function is static.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was wrong. I understood that the function was public and exported. It's ok :-)


/* Parse input from a file and execute it */
int
Expand All @@ -89,6 +90,7 @@ PyRun_InteractiveLoopFlags(FILE *fp, const char *filename_str, PyCompilerFlags *
PyObject *filename, *v;
int ret, err;
PyCompilerFlags local_flags;
int nomem_count = 0;

filename = PyUnicode_DecodeFSDefault(filename_str);
if (filename == NULL) {
Expand All @@ -110,22 +112,32 @@ PyRun_InteractiveLoopFlags(FILE *fp, const char *filename_str, PyCompilerFlags *
_PySys_SetObjectId(&PyId_ps2, v = PyUnicode_FromString("... "));
Py_XDECREF(v);
}
err = -1;
for (;;) {
ret = PyRun_InteractiveOneObject(fp, filename, flags);
err = 0;
do {
ret = PyRun_InteractiveOneObjectEx(fp, filename, flags);
if (ret == -1 && PyErr_Occurred()) {
/* Prevent an endless loop after multiple consecutive MemoryErrors
* while still allowing an interactive command to fail with a
* MemoryError. */
if (PyErr_ExceptionMatches(PyExc_MemoryError)) {
if (++nomem_count > 16) {
PyErr_Clear();
err = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document in Doc/c-api/veryhigh.rst that PyRun_InteractiveLoopFlags() returns a negative number on failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you :-)

break;
}
} else {
nomem_count = 0;
}
PyErr_Print();
flush_io();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to exit at the first MemoryError.

You can write your own code catching MemoryError, like the code module does, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a MemoryError has been raised and when PyRun_InteractiveOneObjectEx() returns, the memory of the frames locals has been released upon unwinding the frames stack and enough memory may be available to continue running the REPL, so it is not clear why the user should not be allowed to enter another command at the next prompt in this case.

BTW it seems that the only module in the standard library (excluding the tests) that catches MemoryError is the linecache module.

} else {
nomem_count = 0;
}
#ifdef Py_REF_DEBUG
if (_PyDebug_XOptionShowRefCount() == Py_True)
_PyDebug_PrintTotalRefs();
#endif
if (ret == E_EOF) {
err = 0;
break;
}
/*
if (ret == E_NOMEM)
break;
*/
}
} while (ret != E_EOF);
Py_DECREF(filename);
return err;
}
Expand Down Expand Up @@ -154,8 +166,11 @@ static int PARSER_FLAGS(PyCompilerFlags *flags)
PyPARSE_WITH_IS_KEYWORD : 0)) : 0)
#endif

int
PyRun_InteractiveOneObject(FILE *fp, PyObject *filename, PyCompilerFlags *flags)
/* A PyRun_InteractiveOneObject() auxiliary function that does not print the
* error on failure. */
static int
PyRun_InteractiveOneObjectEx(FILE *fp, PyObject *filename,
PyCompilerFlags *flags)
{
PyObject *m, *d, *v, *w, *oenc = NULL, *mod_name;
mod_ty mod;
Expand All @@ -167,7 +182,6 @@ PyRun_InteractiveOneObject(FILE *fp, PyObject *filename, PyCompilerFlags *flags)

mod_name = _PyUnicode_FromId(&PyId___main__); /* borrowed */
if (mod_name == NULL) {
PyErr_Print();
return -1;
}

Expand Down Expand Up @@ -227,7 +241,6 @@ PyRun_InteractiveOneObject(FILE *fp, PyObject *filename, PyCompilerFlags *flags)
PyErr_Clear();
return E_EOF;
}
PyErr_Print();
return -1;
}
m = PyImport_AddModuleObject(mod_name);
Expand All @@ -239,15 +252,26 @@ PyRun_InteractiveOneObject(FILE *fp, PyObject *filename, PyCompilerFlags *flags)
v = run_mod(mod, filename, d, d, flags, arena);
PyArena_Free(arena);
if (v == NULL) {
PyErr_Print();
flush_io();
return -1;
}
Py_DECREF(v);
flush_io();
return 0;
}

int
PyRun_InteractiveOneObject(FILE *fp, PyObject *filename, PyCompilerFlags *flags)
{
int res;

res = PyRun_InteractiveOneObjectEx(fp, filename, flags);
if (res == -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we shouldn't call PyErr_Print() + flush_io() in the worst MemoryError case which cleared the error (PyErr_Clear()), since flush_io() is likely to raise a new exception.

What do you think of replacing "if (res == -1)" with "if (res == -1 && PyErr_Occurred())" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may be a confusion: the worst MemoryError case which cleared the error (PyErr_Clear()) occurs in PyRun_InteractiveLoopFlags(), but here we are in PyRun_InteractiveOneObject() and this function does not call PyRun_InteractiveLoopFlags().

The substitution you suggest makes sense, but the initial version of PyRun_InteractiveOneObject() did not call PyErr_Occurred() and I prefer not to change that. What is modified by the PR though, is that now when PyImport_AddModuleObject() returns NULL in PyRun_InteractiveOneObjectEx() then PyErr_Print() is called now while it is not in the initial version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I was confused between the "One" and "Loop" flavors. You can leave it unchanged.

PyErr_Print();
flush_io();
}
return res;
}

int
PyRun_InteractiveOneFlags(FILE *fp, const char *filename_str, PyCompilerFlags *flags)
{
Expand Down