[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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

bpo-39824: module_traverse() don't call m_traverse if md_state=NULL #18738

Merged
merged 4 commits into from
Mar 17, 2020
Merged

bpo-39824: module_traverse() don't call m_traverse if md_state=NULL #18738

merged 4 commits into from
Mar 17, 2020

Conversation

Copy link
Member

vstinner commented Mar 2, 2020

Extension modules: m_traverse, m_clear and m_free functions of
PyModuleDef are no longer called before the module state is
allocated. Extension modules with no module state (m_size <= 0) are
no affected.

https://bugs.python.org/issue39824

Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #18738 into master will increase coverage by 1.08%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #18738       +/-   ##
===========================================
+ Coverage   82.14%   83.22%    +1.08%     
===========================================
  Files        1956     1571      -385     
  Lines      590140   415393   -174747     
  Branches    44488    44489        +1     
===========================================
- Hits       484743   345715   -139028     
+ Misses      95745    60024    -35721     
- Partials     9652     9654        +2     
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-7.15%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.77%) ⬇️
Lib/test/test_asyncio/test_base_events.py 91.84% <0.00%> (-3.30%) ⬇️
... and 435 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28d0bca...fecdcd7. Read the comment docs.

Copy link
Member

shihai1991 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

Comment on lines 201 to 207
This function is not called before the module state is allocated (before
the :c:member:`Py_mod_exec` function is executed): it is not called if
:c:member:`m_size` is greater than 0 and module state
(:c:func:`PyModule_GetState`) is ``NULL``.

.. versionchanged:: 3.9
It is no longer called before the module state is allocated.
Copy link
Member

Choose a reason for hiding this comment

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

IMO this needs clarification.
If a module has m_size of 0 or -1, so the state is not allocated, then m_traverse can be called before the Py_mod_exec function. Right?

Suggested change
This function is not called before the module state is allocated (before
the :c:member:`Py_mod_exec` function is executed): it is not called if
:c:member:`m_size` is greater than 0 and module state
(:c:func:`PyModule_GetState`) is ``NULL``.
.. versionchanged:: 3.9
It is no longer called before the module state is allocated.
This function is not called if module state was requested but is not allocated.
(This is the case immediately after the module is created, before the
:c:member:`Py_mod_exec` function is executed.)
More precisely, this function is not called if :c:member:`m_size` is positive and
module state (as returned by :c:func:`PyModule_GetState`) is ``NULL``.
.. versionchanged:: 3.9
No longer called before the module state is allocated.```

Copy link
Member Author

vstinner commented Mar 3, 2020

@encukou: Thanks for your suggestion. I applied it with minor edits. Would you mind to review the updated PR?

Sorry, I chose to amend the PR rather than add a second commit, to be able to edit the commit message as well (and the NEWS entry).

Copy link
Member Author

vstinner commented Mar 3, 2020

cc @pablogsal @Dormouse759

Extension modules: m_traverse, m_clear and m_free functions of
PyModuleDef are no longer called if the module state was requested
but is not allocated yet. This is the case immediately after the
module is created and before the module is executed (Py_mod_exec
function). More precisely, this function is not called if m_size is
greater than 0 and the module state (as returned by
PyModule_GetState()) is NULL.

Extension modules without module state (m_size <= 0) are not affected.
Copy link
Member Author

I added an entry to What's New In Python 3.9 to advice extension module maintainers to keep an eye on this corner case.

I rebased the PR on master to fix conflicts.

Doc/whatsnew/3.9.rst Outdated Show resolved Hide resolved
Copy link
Member

encukou left a comment

Choose a reason for hiding this comment

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

LGTM, except some grammar in the docs!

vstinner and others added 2 commits March 17, 2020 17:38
Co-Authored-By: Petr Viktorin <encukou@gmail.com>
Co-Authored-By: Petr Viktorin <encukou@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants