Few changes in sys.breakpointhook()

[serhiy-storchaka]

BPO	34756
Nosy	@warsaw, @vstinner, @serhiy-storchaka, @miss-islington
PRs	bpo-34756: Simplify the implementation of sys.breakpointhook(). #9457 [3.7] bpo-34756: Silence only ImportError and AttributeError in sys.breakpointhook(). (GH-9457) #11551 [3.7] bpo-34756: Silence only ImportError and AttributeError in sys.breakpointhook(). (GH-9457) #11551 [3.7] bpo-34756: Silence only ImportError and AttributeError in sys.breakpointhook(). (GH-9457) #11551 bpo-35742: Fix test_envar_unimportable in test_builtin. #11561 bpo-35742: Fix test_envar_unimportable in test_builtin. #11561 [3.7] bpo-35742: Fix test_envar_unimportable in test_builtin. (GH-11561) #11564 [3.7] bpo-35742: Fix test_envar_unimportable in test_builtin. (GH-11561) #11564 [3.7] bpo-35742: Fix test_envar_unimportable in test_builtin. (GH-11561) #11564 [3.7] bpo-35742: Fix test_envar_unimportable in test_builtin. (GH-11561) #11564

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-01-15.11:50:35.213>
created_at = <Date 2018-09-20.17:33:39.264>
labels = ['interpreter-core', 'type-feature', '3.8']
title = 'Few changes in sys.breakpointhook()'
updated_at = <Date 2019-01-15.11:50:35.213>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2019-01-15.11:50:35.213>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2019-01-15.11:50:35.213>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2018-09-20.17:33:39.264>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 34756
keywords = ['patch']
message_count = 10.0
messages = ['325914', '325932', '325969', '325970', '333605', '333606', '333607', '333660', '333678', '333683']
nosy_count = 4.0
nosy_names = ['barry', 'vstinner', 'serhiy.storchaka', 'miss-islington']
pr_nums = ['9457', '11551', '11551', '11551', '11561', '11561', '11564', '11564', '11564', '11564']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue34756'
versions = ['Python 3.8']

[serhiy-storchaka]

The proposed PR makes the following changes in sys.breakpointhook():

• Use _PyObject_GetBuiltin() for getting a builtin. This simplifies the code.

• The only effect of using the "from" list is when the imported name is a submodule. But it should be a callable. Callable module is very rare bird, I don't think we need to support such weird case. Removing the "from" list simplifies the code.

• Only ImportError and AttributeError raised from import are ignored. Other errors are exposed to the user as is. This is most likely a KeyboardInterrupt or MemoryError. They shouldn't be silenced.

sys.breakpointhook() was added in bpo-31353.

[warsaw]

Hi Serhiy. I'm curious whether this is a pure clean up or if there are actual problems you're trying to fix.

• I'm okay with using _PyObject_GetBuiltin() but it does bother me in general to use too many non-public (and thus undocumented) APIs. It makes understanding what's going on more difficult. This is a general gripe of mine with Python's C API.

• Given that removing support for callable modules is technically a backward incompatible change, is it worth it? If we had done it before 3.7 was released, it would have been fine, but now we can't guarantee it won't break someone's code. I agree it's unlikely, but it is possible.

• Perhaps we should explicitly pass through KeyboardInterrupt and MemoryError rather than just ignoring ImportError and AttributeError.

[serhiy-storchaka]

This is a pure clean up except the last item which fixes a minor problem. I had wrote this patch a while ago (perhaps before 3.7.0 was released), and now revive my old patches.

I think that the general rule is that exceptions shouldn't be ignored blindly, except in case when we have no choice (like in destructors). ImportError and AttributeError are expected exceptions raised in PyImport_ImportModuleLevelObject and PyObject_GetAttrString when PYTHONBREAKPOINT points to non-existing name, all other exceptions mean exceptional situation or programming error. Note that exceptions raised when call the hook are not ignored.

[serhiy-storchaka]

I will drop cleanup changes if they don't look good to you.

[serhiy-storchaka]

New changeset 6fe9c44 by Serhiy Storchaka in branch 'master':
bpo-34756: Silence only ImportError and AttributeError in sys.breakpointhook(). (GH-9457)
6fe9c44

[serhiy-storchaka]

The part of the clean up was applied in PR 9519. And since _PyObject_GetBuiltin() was gone, the rest of the clean up no longer applicable. The only part that is left is to make unexpected exceptions no longer silenced.

[miss-islington]

New changeset 6d0254b by Miss Islington (bot) in branch '3.7':
bpo-34756: Silence only ImportError and AttributeError in sys.breakpointhook(). (GH-9457)
6d0254b

[vstinner]

I reopen the issue. This change broke test_builtin: see bpo-35742.

I can reproduce the test failure on my Fedoea 29 using:

./python -m test -v test_builtin -m TestBreakpoint

[serhiy-storchaka]

New changeset 3607ef4 by Serhiy Storchaka in branch 'master':
bpo-35742: Fix test_envar_unimportable in test_builtin. (GH-11561)
3607ef4

[miss-islington]

New changeset 97d6a56 by Miss Islington (bot) in branch '3.7':
bpo-35742: Fix test_envar_unimportable in test_builtin. (GH-11561)
97d6a56