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

xdegaye · 2017-10-28T20:23:09Z

This also fixes partly PyRun_InteractiveOneFlags() that was returning -1 with no exception set.

https://bugs.python.org/issue30696

This also fixes partly ``PyRun_InteractiveOneFlags()`` that was returning -1 with no exception set.

xdegaye · 2017-10-28T20:31:15Z

Could not find a better place than test_readline.py for the test case. The run_pty() function of this module may be moved to test.support but where to put the test case, this is the first test case for the REPL.

vstinner

I dislike retrying on MemoryError. I would prefer to exit immediately.

I also dislike calling Py_FatalError() which we can return an error code.

You may ignore my comments on run_pty(), I didn't notice that you only moved code.

vstinner · 2017-10-30T15:54:42Z

Lib/test/support/__init__.py

+        cleanup.enter_context(proc)
+        def terminate(proc):
+            try:
+                proc.terminate()


Please use kill() instead, no need to be nice if something goes wrong.

Moreover, you should always call proc.wait() after, to prevent zombie process.

FYI exiting the with statement (after the enter_context call) should already guarantee one call to wait. See the bottom of https://docs.python.org/3/library/subprocess.html#subprocess.Popen: “Popen objects are supported as context managers . . . the process is waited for.”

vstinner · 2017-10-30T15:55:54Z

Lib/test/support/__init__.py

+        # to use with select().
+        sel = cleanup.enter_context(selectors.SelectSelector())
+        sel.register(master, selectors.EVENT_READ | selectors.EVENT_WRITE)
+        os.set_blocking(master, False)


I had prefer to set master to non-blocking mode before registering it into the selector.

By the way, a selector must not accept a FD in blocking more :-)

vstinner · 2017-10-30T15:57:45Z

Lib/test/support/__init__.py

+                    output.extend(chunk)
+                if events & selectors.EVENT_WRITE:
+                    try:
+                        input = input[os.write(master, input):]


Please call os.write() on a separated line:

written = os.write(...) input = input[written:]

You might move the "input = ..." in an else block. It's up to you.

vstinner · 2017-10-30T16:05:03Z

Python/pythonrun.c

+                nomem_count = 0;
+            }
+            PyErr_Print();
+            flush_io();


I prefer to exit at the first MemoryError.

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

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.

vstinner · 2017-10-30T16:06:30Z

Python/pythonrun.c

+             * MemoryError. */
+            if (PyErr_ExceptionMatches(PyExc_MemoryError)) {
+                if (++nomem_count > 16) {
+                    Py_FatalError("Cannot recover from MemoryErrors.");


Please don't call Py_FatalError(), but return with an error code.

When returning -1 and exiting at the first MemoryError and there is indeed no memory left upon returning -1, the error message does not indicate that there has been a MemoryError:

$ ./python -q >>> from _testcapi import set_nomemory >>> set_nomemory(0) Error in atexit._run_exitfuncs: Error in atexit._run_exitfuncs: $

In real life Py_FatalError() would also fail to print the error msg since printf() may rely on malloc(). And returning -1 is consistent with the behavior of the other PyRun_*() functions.

I will change PyRun_InteractiveLoopFlags() to return -1 as you have suggested :-)

bedevere-bot · 2017-10-30T16:06:55Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

xdegaye · 2017-11-05T13:36:39Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-05T13:36:41Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

vadmium · 2017-11-06T02:09:43Z

Perhaps you can test with “python -i”, and use pipes rather than a terminal. That should be more portable, avoid platform quirks, give better error handling, be better understood, etc. It looks like there are a few tests in test_cmd_line.py that do this.

vstinner

Thank you for removing the Py_FatalError() call! It's now much better ;-) Second round of review.

vstinner · 2017-11-06T09:47:27Z

Python/pythonrun.c

@@ -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 *);


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.

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

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

vstinner · 2017-11-06T09:48:26Z

Python/pythonrun.c

@@ -89,6 +90,7 @@ PyRun_InteractiveLoopFlags(FILE *fp, const char *filename_str, PyCompilerFlags *
    PyObject *filename, *v;
    int ret, err;
    PyCompilerFlags local_flags;
+    static int nomem_count = 0;


Please add a comment to explain why the variable must be static.

Do we really have to remember the value between two calls?

It does not need to be static, I will correct that.

vstinner · 2017-11-06T09:52:47Z

Python/pythonrun.c

+            if (PyErr_ExceptionMatches(PyExc_MemoryError)) {
+                if (++nomem_count > 16) {
+                    PyErr_Clear();
+                    err = -1;


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

Thank you :-)

vstinner · 2017-11-06T09:56:29Z

Lib/test/support/__init__.py

@@ -2756,6 +2760,60 @@ def fd_count():

    return count

+def run_pty(script, input=b"dummy input\r", env=None):


I propose to add a docstring to document the side effect of import_module('pty'):

"""
Run a script in a temporary pseudoterminal created just for the script.

Raise SkipTest if the pty module cannot be imported.
"""

vstinner · 2017-11-06T09:57:38Z

Lib/test/test_readline.py

@@ -145,11 +141,11 @@ def test_init(self):
 """

    def test_auto_history_enabled(self):
-        output = run_pty(self.auto_history_script.format(True))
+        rc, output = run_pty(self.auto_history_script.format(True))
        self.assertIn(b"History length: 1\r\n", output)


You may add self.assertEqual(rc, 0) in these tests after checking the output.

vstinner · 2017-11-06T10:01:46Z

Lib/test/test_repl.py

+        with SuppressCrashReport():
+            rc, output = run_pty(None, input=user_input)
+        self.assertIn(b'After the exception.\r\n>>>', output)
+        self.assertIn(rc, (1, 120))


Oh, I didn't know this exit code, please add a short comment:

# Exit code 120: Py_FinalizeEx() failed to flush stdout and stderr.

vstinner · 2017-11-06T10:03:10Z

Lib/test/test_repl.py

+
+class TestInteractiveInterpreter(unittest.TestCase):
+
+    def test_no_memory(self):


Please decorate the test with @support.cpython_only, or use support.import_module('_testcapi'), to skip the test on Python implementations without _testcapi.

xdegaye · 2017-11-06T10:25:26Z

@vadmium wrote:

Perhaps you can test with “python -i”, and use pipes rather than a terminal.

In PyRun_AnyFileExFlags(), PyRun_InteractiveLoopFlags() is only called if Py_FdIsInteractive() returns true, so a terminal is required to test that function.

@vadmium also wrote (the following text received from notifications@github.com seems to be lost by github and cannot be found in the PR so I reproduce it here in extenso):

In Lib/test/support/init.py:

+def run_pty(script, input=b"dummy input\r", env=None):
pty = import_module('pty')

output = bytearray()

[master, slave] = pty.openpty()

if script is None:
   args = (sys.executable, '-q')
else:
   args = (sys.executable, '-c', script)
proc = subprocess.Popen(args, stdin=slave, stdout=slave, stderr=slave, env=env)

os.close(slave)

with ExitStack() as cleanup:
   cleanup.enter_context(proc)
   def terminate(proc):
       try:
           proc.terminate()
FYI exiting the with statement (after the enter_context call) should already guarantee one call to wait. See the bottom of https://docs.python.org/3/library/subprocess.html#subprocess.Popen: “Popen objects are supported as context managers . . . the process is waited for.”

Thanks, I will remove the call to proc.wait().

vadmium · 2017-11-06T11:08:02Z

Py_FdIsInteractive also checks Py_InteractiveFlag for special values of filename, and that flag is enabled by “python -i”. Sorry for being persistent, but did you actually try that option out?

xdegaye · 2017-11-06T14:40:46Z

@Haypo and @vadmium
@vadmium wrote:

Py_FdIsInteractive also checks Py_InteractiveFlag for special values of filename, and that flag is enabled by “python -i”. Sorry for being persistent, but did you actually try that option out?

No problem.
I did not try hard enough :-)
You are right and with those last changes, the test runs now using pipes instead of a terminal. So I have reverted the changes made previously to test.support and test_readline and they both remain unchanged by the PR.

xdegaye · 2017-11-06T14:41:32Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-06T14:41:34Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

vstinner

LGTM. Thank you! The PR now looks much better than the first iteration ;-)

I proposed a last change, but I'm not sure that it's worth it/needed.

vstinner · 2017-11-06T15:22:43Z

Python/pythonrun.c

+    int res;
+
+    res = PyRun_InteractiveOneObjectEx(fp, filename, flags);
+    if (res == -1) {


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())" ?

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.

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

miss-islington · 2017-11-12T15:50:51Z

Thanks @xdegaye for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2017-11-12T15:51:07Z

Sorry, @xdegaye, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e0582a37c8d1776a2fd4968e9216f3a05f780276 3.6

bedevere-bot · 2017-11-12T16:08:26Z

GH-4379 is a backport of this pull request to the 3.6 branch.

…). (#4379) (cherry picked from commit e0582a3)

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

560ccee

This also fixes partly ``PyRun_InteractiveOneFlags()`` that was returning -1 with no exception set.

xdegaye added needs backport to 3.6 type-bug An unexpected behavior, bug, or error labels Oct 28, 2017

xdegaye requested a review from vstinner October 28, 2017 20:23

the-knights-who-say-ni added the CLA signed label Oct 28, 2017

bedevere-bot added the awaiting merge label Oct 28, 2017

xdegaye added 4 commits October 29, 2017 14:04

Do not break backward compatibility

15230d1

Call _PyDebug_PrintTotalRefs() after printing the exception

ac6003a

Create test_repl.py and move run_pty() to test.support

44c335e

Update Misc/NEWS.

be1df91

vstinner requested changes Oct 30, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Oct 30, 2017

xdegaye added 2 commits November 5, 2017 13:53

Do not call Py_FatalError() and return an error

477f9c5

Merge remote-tracking branch 'origin/master' into bpo-30696

1516d4c

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 5, 2017

vstinner reviewed Nov 6, 2017

View reviewed changes

Updates after the second round of review

1fe98b2

vstinner approved these changes Nov 6, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 6, 2017

xdegaye merged commit e0582a3 into python:master Nov 12, 2017

bedevere-bot removed the awaiting merge label Nov 12, 2017

bedevere-bot removed the needs backport to 3.6 label Nov 12, 2017

xdegaye added a commit that referenced this pull request Nov 12, 2017

[3.6] bpo-30696: Fix the REPL looping endlessly when no memory (GH-4160…

ea5b545

…). (#4379) (cherry picked from commit e0582a3)

xdegaye deleted the bpo-30696 branch January 19, 2018 15:25

		@@ -2756,6 +2760,60 @@ def fd_count():

		return count

		def run_pty(script, input=b"dummy input\r", env=None):


		class TestInteractiveInterpreter(unittest.TestCase):

		def test_no_memory(self):

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

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

Conversation

xdegaye commented Oct 28, 2017 • edited by bedevere-bot Loading

xdegaye commented Oct 28, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 30, 2017

xdegaye commented Nov 5, 2017

bedevere-bot commented Nov 5, 2017

vadmium commented Nov 6, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xdegaye commented Nov 6, 2017

vadmium commented Nov 6, 2017

xdegaye commented Nov 6, 2017

xdegaye commented Nov 6, 2017

bedevere-bot commented Nov 6, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Nov 12, 2017

miss-islington commented Nov 12, 2017

bedevere-bot commented Nov 12, 2017

xdegaye commented Oct 28, 2017 •

edited by bedevere-bot

Loading