Skip to content

Conversation

@vstinner
Copy link
Member

@vstinner 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

@codecov
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.

Impacted file tree graph

@@             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 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
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.```

@vstinner
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).

@vstinner
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.
@vstinner
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.

Copy link
Member

@encukou 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!

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