bpo-41631: _ast module uses again a global state #21961

vstinner · 2020-08-26T09:26:15Z

Using a different state per module instance is causing subtle
practical problems.

For example, the Mercurial project replaces the import() function
to implement lazy import, whereas Python expected that "import _ast"
always return a fully initialized _ast module.

https://bugs.python.org/issue41631

vstinner · 2020-08-26T09:27:01Z

cc @pablogsal @corona10 @encukou

vstinner · 2020-08-26T09:31:08Z

Hum, I'm not sure that this change is correct. If a second instance of the _ast module is created, astmodule_clear() will clear the state which makes the first instance unusable :-(

vstinner · 2020-08-26T09:37:12Z

I also reverted partially the commit b1cc6ba "Convert _ast extension to PEP 489": astmodule_clear() and astmodule_free() now do nothing, the global _ast module state is never cleared.

corona10 · 2020-08-26T13:26:15Z

Parser/asdl_c.py

@@ -1473,6 +1452,7 @@ def write_source(f, mod):
    f.write('#include "Python.h"\n')
    f.write('#include "%s-ast.h"\n' % mod.name)
    f.write('#include "structmember.h"         // PyMemberDef\n')
+    f.write('#include "pycore_pylifecycle.h"   // export _PyAST_Fini()\n')


This code line occurs Py_BUILD_CORE error

It seems like extern void _PyAST_Fini(PyThreadState *tstate); is not needed to export the symbol. I just removed the include.

encukou · 2020-08-26T23:17:40Z

Unfortunately, now all the node types are heap types shared across interpreters.
An arbitrary attribute can be set on an AST node class to smuggle any object to another interpreter.

Can they be made static again? Or maybe the AST state can be part of interpreter state.

encukou · 2020-09-02T15:41:35Z

@vstinner @pablogsal: I've made the module state per-interpreter, i.e. part of the interpreter state, here: vstinner/cpython@revert_ast...encukou:revert_ast

The complication is that Python-ast.c is also used to build extension modules by pegen tests. See pegen/ast_dump.py for one note about this. Since there can be several such modules, they can't really use per-interpreter state. My solution is to make their state global, and make such modules loadable only once per process to avoid sharing the state.

@vstinner, do you want to use that commit for this PR?

vstinner · 2020-09-11T09:30:44Z

I updated my PR to convert AST_type again to a static type to fix a corner case with subinterpreters: https://bugs.python.org/issue41631#msg376686

cc @encukou

vstinner · 2020-09-11T09:34:57Z

@pablogsal @corona10: Would you mind to review this PR? It fix a release blocker for Python 3.9.0, a regression compared to 3.8: see https://bugs.python.org/issue41631 The deadline for 3.9.0rc2 is next monday :-(

@encukou proposes PR #21973 to workaround the regression and to fix the regression in a 3.9 bugfix release (like 3.9.1).

This PR is non-trivial, but _ast got tons of changes between 3.8 and 3.9, and I would prefer to avoid further large changes (like this PR) in the 3.9 stable branch after 3.9.0 final release.

I'm not sure what is the best option :-(

I suggest you to revert the two commits separately, since they are non-trivial:
https://github.com/python/cpython/pull/21961/commits

Note: If we choose to merge this PR and backport it to Python 3.9, do you think that it would be better to merge the second commit in a separated PR, since commits of this PR will be squashed?

vstinner · 2020-09-11T09:50:18Z

Summary of my change: this PR reverts changes made after 3.8, to move back to the battle-tested 3.8 code. I reverted the changes which causes many bugs.

encukou · 2020-09-11T09:54:20Z

@encukou proposes PR #21973 to workaround the regression and to fix the regression in a 3.9 bugfix release (like 3.9.1).

Not necessarily in 3.9.1 – I'd be OK with fixing this in 3.10 as well.

vstinner · 2020-09-11T10:22:09Z

Not necessarily in 3.9.1 – I'd be OK with fixing this in 3.10 as well.

If possible, I would prefer to https://bugs.python.org/issue41631 regression in Python 3.9.x (the best would be to fix it in 3.9.0).

vstinner · 2020-09-11T10:45:35Z

I fixed the Windows build, but now the documentation job fails with:

Warning, treated as error:

/home/travis/build/python/cpython/Doc/library/string.rst:311:duplicate token description of format_spec, other instance in reference/lexical_analysis

It seems to be unrelated to my change: https://bugs.python.org/issue41762

corona10

LGTM, I locally run this PR and it looks good.

vstinner · 2020-09-11T12:42:59Z

Close/reopen to attempt to magically fix https://bugs.python.org/issue41762

pablogsal · 2020-09-11T23:01:28Z

@pablogsal @corona10: Would you mind to review this PR? It fix a release blocker for Python 3.9.0, a regression compared to 3.8: see https://bugs.python.org/issue41631 The deadline for 3.9.0rc2 is next monday :-(

@encukou proposes PR #21973 to workaround the regression and to fix the regression in a 3.9 bugfix release (like 3.9.1).

This PR is non-trivial, but _ast got tons of changes between 3.8 and 3.9, and I would prefer to avoid further large changes (like this PR) in the 3.9 stable branch after 3.9.0 final release.

I'm not sure what is the best option :-(

I suggest you to revert the two commits separately, since they are non-trivial:
https://github.com/python/cpython/pull/21961/commits

Note: If we choose to merge this PR and backport it to Python 3.9, do you think that it would be better to merge the second commit in a separated PR, since commits of this PR will be squashed?

I tried to review reviewed this today but time ran out so I will try to do a review tomorrow. In any case, it would be good to have regression test for the problems we have uncovered.

encukou · 2020-09-14T09:50:43Z

My PR has a regression test; feel free to adapt it: https://github.com/python/cpython/pull/21973/files#diff-f265a39e24fab89dc68309a7db18e171R1948

Partially revert commit ac46eb4: "bpo-38113: Update the Python-ast.c generator to PEP384 (gh-15957)". Using a module state per module instance is causing subtle practical problems. For example, the Mercurial project replaces the __import__() function to implement lazy import, whereas Python expected that "import _ast" always return a fully initialized _ast module. Add _PyAST_Fini() to clear the state at exit. The _ast module has no state (set _astmodule.m_size to 0). Remove astmodule_traverse(), astmodule_clear() and astmodule_free() functions.

encukou · 2020-09-15T13:23:17Z

This does not really move _ast back battle-tested 3.8 code. It does not revert everything, so it possibly has new bugs.

One of the bugs was already present in 3.8 – it's possible to objects across subinterpreters. We can agree to ignore that in 3.9.0\ and say that multiple interpreters still aren't safe. That's fine with me at this point; I opened ericsnowcurrently/multi-core-python#68 to track this.

But then, the second commit here doesn't solve anything: it makes only one of the types static, when are lots of heap types still remaining. Or is it supposed to solve a different problem?

If not, I say we should merge only the first commit here, plus any tests.

vstinner · 2020-09-15T13:30:36Z

@pablogsal: I added 3 tests to test_ast which should cover all bugs listed in https://bugs.python.org/issue41631

With the new subinterpreter test, I noticed two reference leak bugs which are not introduced by my PR, but simply have not be seen previously. So I fixed them as part of this PR as well. Otherwise, this PR would make all Refleak buildbot failing.

vstinner · 2020-09-15T13:39:59Z

One of the bugs was already present in 3.8 – it's possible to objects across subinterpreters. We can agree to ignore that in 3.9.0\ and say that multiple interpreters still aren't safe. That's fine with me at this point; I opened ericsnowcurrently/multi-core-python#68 to track this.

You are correct that _ast.AST subclasses like _ast.Constant can be modified and changes impact all interpreters.

I attempted to better isolate the _ast module in subinterpreter (fix ericsnowcurrently/multi-core-python#68 ). My intent was to fix some bugs by doing that, but I introduced other new bugs :-(

Example:

import _ast
from test.support import run_in_subinterp
run_in_subinterp('import _ast; _ast.Constant.x=1')
print(_ast.Constant.x)

This code displays 1 with Python 3.8 and with my PR (master branch): same behavior (not better, not worse).

Since it's not a regression compared to Python 3.8, I suggest to open a new issue if you consider that the _ast module should be enhanced.

But then, the second commit here doesn't solve anything: it makes only one of the types static, when are lots of heap types still remaining. Or is it supposed to solve a different problem?

My intent is to move back to Python 3.8 status quo (behavior) for the _ast.AST type. For example, with my change, it's not possible to add a new attribute to _ast.AST in the main interpreter or in a subinterpreter.

Example:

import _ast
_ast.AST.x = 1

This code raises TypeError: can't set attributes of built-in/extension type 'ast.AST' with Python 3.8 and with my PR (not better, not worse).

vstinner · 2020-09-15T13:43:51Z

My intent is to move back to Python 3.8 status quo (behavior) for the _ast.AST type. For example, with my change, it's not possible to add a new attribute to _ast.AST in the main interpreter or in a subinterpreter.

I added a 4th test to check this behavior with the comment:

# [bpo-41631](https://bugs.python.org/issue41631): The _ast.AST type is static and cannot be modified.
# This test should be removed when the _ast.AST type will be converted
# to a heap type.

encukou · 2020-09-15T13:49:19Z

not better, not worse

Right. Commits 2 and 3 in this PR are not necessary.

To me, reverting to 3.8 would make sense as a reason if _ast was reverted all the way back to how it was in 3.8. But that's not the case here, so I don't see a reason for making _ast.AST static. IMO, it's better to stay closer to 3.9.0.rc1.

vstinner · 2020-09-15T14:04:16Z

Right. Commits 2 and 3 in this PR are not necessary.

My first intent was to prevent changes of the _ast module in a subinterpreter which would impact other interpreters.

Sadly, I didn't notice that it was already possible to modify other interpreter using _ast.AST subtypes.

By the way, import ast does modify ast.Constant and ast.Tuple types on purpose, to add deprecated methods.

vstinner · 2020-09-15T14:06:28Z

I'm fine with leaving _ast.AST as a heap type in Python 3.9 (remove my commits 2 and 3), and so keep this minor behavior change. It can be fixed later. As I wrote, there was already other ways to change the state of other interpreters using _ast.AST subtypes.

Do other people have an opinion on that?

corona10

Since this PR is a blocker,
I would like to approve this PR if the mercurial issue is solved and other regression.
(And looks like this issue is solved)

And other minor behavior can be modified after this release.

* Only call _PyAST_Fini() in the main interpreter * "import ast" only adds deprecated methods once to _ast.Constant and _ast.Tuple types.

vstinner · 2020-09-15T15:06:40Z

I removed the commits which convert _ast.AST back to a static type.

bedevere-bot · 2020-09-15T16:03:38Z

@ambv: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2020-09-15T16:03:38Z

Thanks @vstinner for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2020-09-15T16:03:53Z

Sorry, @vstinner and @ambv, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e5fbe0cbd4be99ced5f000ad382208ad2a561c90 3.9

Partially revert commit ac46eb4: "bpo-38113: Update the Python-ast.c generator to PEP384 (pythongh-15957)". Using a module state per module instance is causing subtle practical problems. For example, the Mercurial project replaces the __import__() function to implement lazy import, whereas Python expected that "import _ast" always return a fully initialized _ast module. Add _PyAST_Fini() to clear the state at exit. The _ast module has no state (set _astmodule.m_size to 0). Remove astmodule_traverse(), astmodule_clear() and astmodule_free() functions.. (cherry picked from commit e5fbe0c) Co-authored-by: Victor Stinner <vstinner@python.org>

bedevere-bot · 2020-09-15T18:07:56Z

GH-22258 is a backport of this pull request to the 3.9 branch.

…-22258) Partially revert commit ac46eb4: "bpo-38113: Update the Python-ast.c generator to PEP384 (gh-15957)". Using a module state per module instance is causing subtle practical problems. For example, the Mercurial project replaces the __import__() function to implement lazy import, whereas Python expected that "import _ast" always return a fully initialized _ast module. Add _PyAST_Fini() to clear the state at exit. The _ast module has no state (set _astmodule.m_size to 0). Remove astmodule_traverse(), astmodule_clear() and astmodule_free() functions.. (cherry picked from commit e5fbe0c) Co-authored-by: Victor Stinner <vstinner@python.org>

Partially revert commit ac46eb4: "bpo-38113: Update the Python-ast.c generator to PEP384 (pythongh-15957)". Using a module state per module instance is causing subtle practical problems. For example, the Mercurial project replaces the __import__() function to implement lazy import, whereas Python expected that "import _ast" always return a fully initialized _ast module. Add _PyAST_Fini() to clear the state at exit. The _ast module has no state (set _astmodule.m_size to 0). Remove astmodule_traverse(), astmodule_clear() and astmodule_free() functions.

vstinner requested review from lysnikolaou and pablogsal as code owners August 26, 2020 09:26

the-knights-who-say-ni added the CLA signed label Aug 26, 2020

bedevere-bot added the awaiting core review label Aug 26, 2020

vstinner added the needs backport to 3.9 only security fixes label Aug 26, 2020

corona10 self-requested a review August 26, 2020 11:00

corona10 reviewed Aug 26, 2020

View reviewed changes

encukou mentioned this pull request Aug 26, 2020

bpo-41631: Check that importing _ast returns the right module #21973

Closed

corona10 approved these changes Sep 11, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 11, 2020

vstinner closed this Sep 11, 2020

vstinner reopened this Sep 11, 2020

corona10 approved these changes Sep 15, 2020

View reviewed changes

vstinner added 2 commits September 15, 2020 17:05

Add tests

3f42a36

Fix reference leaks in ast and _ast modules

d5c5f2e

* Only call _PyAST_Fini() in the main interpreter * "import ast" only adds deprecated methods once to _ast.Constant and _ast.Tuple types.

ambv merged commit e5fbe0c into python:master Sep 15, 2020

bedevere-bot removed the awaiting merge label Sep 15, 2020

miss-islington assigned ambv Sep 15, 2020

bedevere-bot removed the needs backport to 3.9 only security fixes label Sep 15, 2020

vstinner deleted the revert_ast branch September 16, 2020 10:04

neonene mentioned this pull request May 10, 2024

Isolate the _datetime extension module #117398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-41631: _ast module uses again a global state #21961

bpo-41631: _ast module uses again a global state #21961

vstinner commented Aug 26, 2020 •

edited by bedevere-bot

Loading

vstinner commented Aug 26, 2020

vstinner commented Aug 26, 2020

vstinner commented Aug 26, 2020

corona10 Aug 26, 2020

vstinner Aug 26, 2020

encukou commented Aug 26, 2020

encukou commented Sep 2, 2020

vstinner commented Sep 11, 2020

vstinner commented Sep 11, 2020

vstinner commented Sep 11, 2020

encukou commented Sep 11, 2020

vstinner commented Sep 11, 2020

vstinner commented Sep 11, 2020

corona10 left a comment

vstinner commented Sep 11, 2020

pablogsal commented Sep 11, 2020

encukou commented Sep 14, 2020

encukou commented Sep 15, 2020

vstinner commented Sep 15, 2020

vstinner commented Sep 15, 2020

vstinner commented Sep 15, 2020 •

edited by bedevere-bot

Loading

encukou commented Sep 15, 2020

vstinner commented Sep 15, 2020

vstinner commented Sep 15, 2020

corona10 left a comment

vstinner commented Sep 15, 2020

bedevere-bot commented Sep 15, 2020

miss-islington commented Sep 15, 2020

miss-islington commented Sep 15, 2020

bedevere-bot commented Sep 15, 2020

bpo-41631: _ast module uses again a global state #21961

bpo-41631: _ast module uses again a global state #21961

Conversation

vstinner commented Aug 26, 2020 • edited by bedevere-bot Loading

vstinner commented Aug 26, 2020

vstinner commented Aug 26, 2020

vstinner commented Aug 26, 2020

corona10 Aug 26, 2020

Choose a reason for hiding this comment

vstinner Aug 26, 2020

Choose a reason for hiding this comment

encukou commented Aug 26, 2020

encukou commented Sep 2, 2020

vstinner commented Sep 11, 2020

vstinner commented Sep 11, 2020

vstinner commented Sep 11, 2020

encukou commented Sep 11, 2020

vstinner commented Sep 11, 2020

vstinner commented Sep 11, 2020

corona10 left a comment

Choose a reason for hiding this comment

vstinner commented Sep 11, 2020

pablogsal commented Sep 11, 2020

encukou commented Sep 14, 2020

encukou commented Sep 15, 2020

vstinner commented Sep 15, 2020

vstinner commented Sep 15, 2020

vstinner commented Sep 15, 2020 • edited by bedevere-bot Loading

encukou commented Sep 15, 2020

vstinner commented Sep 15, 2020

vstinner commented Sep 15, 2020

corona10 left a comment

Choose a reason for hiding this comment

vstinner commented Sep 15, 2020

bedevere-bot commented Sep 15, 2020

miss-islington commented Sep 15, 2020

miss-islington commented Sep 15, 2020

bedevere-bot commented Sep 15, 2020

vstinner commented Aug 26, 2020 •

edited by bedevere-bot

Loading

vstinner commented Sep 15, 2020 •

edited by bedevere-bot

Loading