-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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.
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
gh-112292 : Catch import error conditions with readline hooks #112313
gh-112292 : Catch import error conditions with readline hooks #112313
Conversation
+1 to fixing the other hooks, whether in this PR or another (though I'd think this PR would be the better place). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. I've noted one thing to fix and a couple others to consider.
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 |
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
… into robust_readline_hook
… to get global state
… checks (won't raise assertion in non-debug code)
I have made the requested changes; please review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra effort here. There are a few things we may want to do differently here, for which I've left notes.
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
… into robust_readline_hook
@ericsnowcurrently it wasn't possible to check module state inside $ ./python.exe test_in_interp.py test_builtin
Running test test_builtin
test_abs (test.test_builtin.BuiltinTest.test_abs) ... ok
test_all (test.test_builtin.BuiltinTest.test_all) ... ok
test_any (test.test_builtin.BuiltinTest.test_any) ... ok
test_ascii (test.test_builtin.BuiltinTest.test_ascii) ... ok
test_bin (test.test_builtin.BuiltinTest.test_bin) ... ok
test_bug_27936 (test.test_builtin.BuiltinTest.test_bug_27936) ... ok
test_bytearray_extend_error (test.test_builtin.BuiltinTest.test_bytearray_extend_error) ... ok
test_bytearray_translate (test.test_builtin.BuiltinTest.test_bytearray_translate) ... ok
test_callable (test.test_builtin.BuiltinTest.test_callable) ... ok
test_chr (test.test_builtin.BuiltinTest.test_chr) ... ok
test_cmp (test.test_builtin.BuiltinTest.test_cmp) ... ok
test_compile (test.test_builtin.BuiltinTest.test_compile) ... ok
test_compile_ast (test.test_builtin.BuiltinTest.test_compile_ast) ... ok
test_compile_async_generator (test.test_builtin.BuiltinTest.test_compile_async_generator)
With the PyCF_ALLOW_TOP_LEVEL_AWAIT flag added in 3.8, we want to ... ok
test_compile_top_level_await (test.test_builtin.BuiltinTest.test_compile_top_level_await)
Test whether code some top level await can be compiled. ... ok
test_compile_top_level_await_invalid_cases (test.test_builtin.BuiltinTest.test_compile_top_level_await_invalid_cases) ... ok
test_compile_top_level_await_no_coro (test.test_builtin.BuiltinTest.test_compile_top_level_await_no_coro)
Make sure top level non-await codes get the correct coroutine flags ... ok
test_construct_singletons (test.test_builtin.BuiltinTest.test_construct_singletons) ... ok
test_delattr (test.test_builtin.BuiltinTest.test_delattr) ... ok
test_dir (test.test_builtin.BuiltinTest.test_dir) ... ok
test_divmod (test.test_builtin.BuiltinTest.test_divmod) ... ok
test_eval (test.test_builtin.BuiltinTest.test_eval) ... ok
test_exec (test.test_builtin.BuiltinTest.test_exec) ... ok
test_exec_closure (test.test_builtin.BuiltinTest.test_exec_closure) ... ok
test_exec_globals (test.test_builtin.BuiltinTest.test_exec_globals) ... ok
test_exec_globals_dict_subclass (test.test_builtin.BuiltinTest.test_exec_globals_dict_subclass) ... ok
test_exec_globals_error_on_get (test.test_builtin.BuiltinTest.test_exec_globals_error_on_get) ... ok
test_exec_globals_frozen (test.test_builtin.BuiltinTest.test_exec_globals_frozen) ... ok
test_exec_redirected (test.test_builtin.BuiltinTest.test_exec_redirected) ... ok
test_filter (test.test_builtin.BuiltinTest.test_filter) ... ok
test_filter_dealloc (test.test_builtin.BuiltinTest.test_filter_dealloc) ... skipped "resource 'cpu' is not enabled"
test_filter_pickle (test.test_builtin.BuiltinTest.test_filter_pickle) ... ok
test_format (test.test_builtin.BuiltinTest.test_format) ... ok
test_general_eval (test.test_builtin.BuiltinTest.test_general_eval) ... ok
test_getattr (test.test_builtin.BuiltinTest.test_getattr) ... ok
test_hasattr (test.test_builtin.BuiltinTest.test_hasattr) ... ok
test_hash (test.test_builtin.BuiltinTest.test_hash) ... ok
test_hex (test.test_builtin.BuiltinTest.test_hex) ... ok
test_id (test.test_builtin.BuiltinTest.test_id) ... ok
test_import (test.test_builtin.BuiltinTest.test_import) ... ok
test_input (test.test_builtin.BuiltinTest.test_input) ... ok
test_isinstance (test.test_builtin.BuiltinTest.test_isinstance) ... ok
test_issubclass (test.test_builtin.BuiltinTest.test_issubclass) ... ok
test_iter (test.test_builtin.BuiltinTest.test_iter) ... ok
test_len (test.test_builtin.BuiltinTest.test_len) ... ok
test_map (test.test_builtin.BuiltinTest.test_map) ... ok
test_map_pickle (test.test_builtin.BuiltinTest.test_map_pickle) ... ok
test_max (test.test_builtin.BuiltinTest.test_max) ... ok
test_min (test.test_builtin.BuiltinTest.test_min) ... ok
test_neg (test.test_builtin.BuiltinTest.test_neg) ... ok
test_next (test.test_builtin.BuiltinTest.test_next) ... ok
test_oct (test.test_builtin.BuiltinTest.test_oct) ... ok
test_open (test.test_builtin.BuiltinTest.test_open) ... ok
test_open_default_encoding (test.test_builtin.BuiltinTest.test_open_default_encoding) ... ok
test_open_non_inheritable (test.test_builtin.BuiltinTest.test_open_non_inheritable) ... ok
test_ord (test.test_builtin.BuiltinTest.test_ord) ... ok
test_pow (test.test_builtin.BuiltinTest.test_pow) ... ok
test_repr (test.test_builtin.BuiltinTest.test_repr) ... ok
test_round (test.test_builtin.BuiltinTest.test_round) ... ok
test_round_large (test.test_builtin.BuiltinTest.test_round_large) ... ok
test_setattr (test.test_builtin.BuiltinTest.test_setattr) ... ok
test_sum (test.test_builtin.BuiltinTest.test_sum) ... ok
test_sum_accuracy (test.test_builtin.BuiltinTest.test_sum_accuracy) ... ok
test_type (test.test_builtin.BuiltinTest.test_type) ... ok
test_vars (test.test_builtin.BuiltinTest.test_vars) ... ok
test_warning_notimplemented (test.test_builtin.BuiltinTest.test_warning_notimplemented) ... ok
test_zip (test.test_builtin.BuiltinTest.test_zip) ... ok
test_zip_bad_iterable (test.test_builtin.BuiltinTest.test_zip_bad_iterable) ... ok
test_zip_pickle (test.test_builtin.BuiltinTest.test_zip_pickle) ... ok
test_zip_pickle_strict (test.test_builtin.BuiltinTest.test_zip_pickle_strict) ... ok
test_zip_pickle_strict_fail (test.test_builtin.BuiltinTest.test_zip_pickle_strict_fail) ... ok
test_zip_result_gc (test.test_builtin.BuiltinTest.test_zip_result_gc) ... ok
test_zip_strict (test.test_builtin.BuiltinTest.test_zip_strict) ... ok
test_zip_strict_error_handling (test.test_builtin.BuiltinTest.test_zip_strict_error_handling) ... ok
test_zip_strict_error_handling_stopiteration (test.test_builtin.BuiltinTest.test_zip_strict_error_handling_stopiteration) ... ok
test_zip_strict_iterators (test.test_builtin.BuiltinTest.test_zip_strict_iterators) ... ok
test_immortals (test.test_builtin.ImmortalTests.test_immortals) ... ok
test_list_repeat_respect_immortality (test.test_builtin.ImmortalTests.test_list_repeat_respect_immortality) ... ok
test_tuple_repeat_respect_immortality (test.test_builtin.ImmortalTests.test_tuple_repeat_respect_immortality) ... ok
test_input_no_stdout_fileno (test.test_builtin.PtyTests.test_input_no_stdout_fileno) ... ERROR
test_input_tty (test.test_builtin.PtyTests.test_input_tty) ... ERROR
test_input_tty_non_ascii (test.test_builtin.PtyTests.test_input_tty_non_ascii) ... ERROR
test_input_tty_non_ascii_unicode_errors (test.test_builtin.PtyTests.test_input_tty_non_ascii_unicode_errors) ... ERROR
test_cleanup (test.test_builtin.ShutdownTest.test_cleanup) ... ok
test_breakpoint (test.test_builtin.TestBreakpoint.test_breakpoint) ... ok
test_breakpoint_with_args_and_keywords (test.test_builtin.TestBreakpoint.test_breakpoint_with_args_and_keywords) ... ok
test_breakpoint_with_breakpointhook_reset (test.test_builtin.TestBreakpoint.test_breakpoint_with_breakpointhook_reset) ... ok
test_breakpoint_with_breakpointhook_set (test.test_builtin.TestBreakpoint.test_breakpoint_with_breakpointhook_set) ... ok
test_breakpoint_with_passthru_error (test.test_builtin.TestBreakpoint.test_breakpoint_with_passthru_error) ... ok
test_envar_good_path_builtin (test.test_builtin.TestBreakpoint.test_envar_good_path_builtin) ... ok
test_envar_good_path_empty_string (test.test_builtin.TestBreakpoint.test_envar_good_path_empty_string) ... ok
test_envar_good_path_noop_0 (test.test_builtin.TestBreakpoint.test_envar_good_path_noop_0) ... ok
test_envar_good_path_other (test.test_builtin.TestBreakpoint.test_envar_good_path_other) ... ok
test_envar_ignored_when_hook_is_set (test.test_builtin.TestBreakpoint.test_envar_ignored_when_hook_is_set) ... ok
test_envar_unimportable (test.test_builtin.TestBreakpoint.test_envar_unimportable) ... ok
test_runtime_error_when_hook_is_lost (test.test_builtin.TestBreakpoint.test_runtime_error_when_hook_is_lost) ... ok
test_bad_arguments (test.test_builtin.TestSorted.test_bad_arguments) ... ok
test_baddecorator (test.test_builtin.TestSorted.test_baddecorator) ... ok
test_basic (test.test_builtin.TestSorted.test_basic) ... ok
test_inputtypes (test.test_builtin.TestSorted.test_inputtypes) ... ok
test_bad_args (test.test_builtin.TestType.test_bad_args) ... ok
test_bad_slots (test.test_builtin.TestType.test_bad_slots) ... ok
test_namespace_order (test.test_builtin.TestType.test_namespace_order) ... ok
test_new_type (test.test_builtin.TestType.test_new_type) ... ok
test_type_doc (test.test_builtin.TestType.test_type_doc) ... ok
test_type_name (test.test_builtin.TestType.test_type_name) ... ok
test_type_nokwargs (test.test_builtin.TestType.test_type_nokwargs) ... ok
test_type_qualname (test.test_builtin.TestType.test_type_qualname) ... ok
test_type_typeparams (test.test_builtin.TestType.test_type_typeparams) ... ok
bin (builtins)
Doctest: builtins.bin ... ok
hex (builtins.bytearray)
Doctest: builtins.bytearray.hex ... ok
hex (builtins.bytes)
Doctest: builtins.bytes.hex ... ok
as_integer_ratio (builtins.float)
Doctest: builtins.float.as_integer_ratio ... ok
fromhex (builtins.float)
Doctest: builtins.float.fromhex ... ok
hex (builtins.float)
Doctest: builtins.float.hex ... ok
hex (builtins)
Doctest: builtins.hex ... ok
int (builtins)
Doctest: builtins.int ... ok
as_integer_ratio (builtins.int)
Doctest: builtins.int.as_integer_ratio ... ok
bit_count (builtins.int)
Doctest: builtins.int.bit_count ... ok
bit_length (builtins.int)
Doctest: builtins.int.bit_length ... ok
hex (builtins.memoryview)
Doctest: builtins.memoryview.hex ... ok
oct (builtins)
Doctest: builtins.oct ... ok
zip (builtins)
Doctest: builtins.zip ... ok
======================================================================
ERROR: test_input_no_stdout_fileno (test.test_builtin.PtyTests.test_input_no_stdout_fileno)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2314, in test_input_no_stdout_fileno
lines = self.run_child(child, b"quux\r")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter
======================================================================
ERROR: test_input_tty (test.test_builtin.PtyTests.test_input_tty)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2284, in test_input_tty
self.check_input_tty("prompt", b"quux")
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2269, in check_input_tty
lines = self.run_child(child, terminal_input + b"\r\n")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter
======================================================================
ERROR: test_input_tty_non_ascii (test.test_builtin.PtyTests.test_input_tty_non_ascii)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2299, in test_input_tty_non_ascii
self.check_input_tty("prompté", b"quux\xe9", "utf-8")
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2269, in check_input_tty
lines = self.run_child(child, terminal_input + b"\r\n")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter
======================================================================
ERROR: test_input_tty_non_ascii_unicode_errors (test.test_builtin.PtyTests.test_input_tty_non_ascii_unicode_errors)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2304, in test_input_tty_non_ascii_unicode_errors
self.check_input_tty("prompté", b"quux\xe9", "ascii")
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2269, in check_input_tty
lines = self.run_child(child, terminal_input + b"\r\n")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/test/test_builtin.py", line 2187, in run_child
old_sighup = signal.signal(signal.SIGHUP, self.handle_sighup)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/anthonyshaw/projects/cpython/Lib/signal.py", line 56, in signal
handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: signal only works in main thread of the main interpreter
----------------------------------------------------------------------
Ran 122 tests in 0.298s
FAILED (errors=4, skipped=1)
test test_builtin failed
$ ./python.exe test_in_interp.py test_readline
Running test test_readline
test_readline skipped -- module readline does not support loading in subinterpreters |
I'm guessing it's because
Yeah, that's the higher priority thing. The module is definitely in a better place now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for taking care of this, @tonybaloney! |
How much of this should be backported to 3.12? |
All of it could be |
@Yhg1s, what do you think about backporting this as-is? |
…ythongh-112313) Prevents a segmentation fault in registered hooks for the readline library, but only when the readline module is loaded inside an isolated sub interpreter. The module is single-phase init so loading it fails, but not until the module init function has already run, where the readline hooks get registered. The readlinestate_global macro was error-prone to PyImport_FindModule returning NULL and crashing in about 18 places. I could reproduce 1 easily, but this PR replaces the macro with a function and adds error conditions to the other functions.
…ythongh-112313) Prevents a segmentation fault in registered hooks for the readline library, but only when the readline module is loaded inside an isolated sub interpreter. The module is single-phase init so loading it fails, but not until the module init function has already run, where the readline hooks get registered. The readlinestate_global macro was error-prone to PyImport_FindModule returning NULL and crashing in about 18 places. I could reproduce 1 easily, but this PR replaces the macro with a function and adds error conditions to the other functions.
Prevents a segmentation fault when modules import (or configure)
readline
from inside a sub interpreter.The
readlinestate_global
macro was error-prone toPyImport_FindModule
returning NULL and crashing in about 18 places. I could reproduce 1 easily, but this PR replaces the macro with a function and adds error conditions to the other functions.