bpo-1635741: Port _locale extension module to multiphase initialization (PEP 489) by shihai1991 · Pull Request #18358 · python/cpython · GitHub

https://bugs.python.org/issue1635741

Codecov Report

Merging #18358 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18358      +/-   ##
==========================================
- Coverage   82.11%   82.07%   -0.05%     
==========================================
  Files        1955     1955              
  Lines      588624   584187    -4437     
  Branches    44406    44464      +58     
==========================================
- Hits       483366   479456    -3910     
+ Misses      95615    95110     -505     
+ Partials     9643     9621      -22

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%)	⬇️
Modules/_decimal/libmpdec/umodarith.h	80.76% <0.00%> (-19.24%)	⬇️
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%)	⬇️
... and 399 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 850a4bd...f3a1d08. Read the comment docs.

I don't think that it's worth it to check for error: this function cannot fail, self is always a module object.

I would prefer a function like:

static inline _locale_state* get_locale_state(PyObject *mod)
{
    void *state = PyModule_GetState(mod);
    assert(state != NULL);
    return (_locale_state*)state;
}

And then use:

PyErr_SetString(get_locale_state(self)->Error, "invalid locale category");

Make sense, I update this PR.

Do we need add a global macros of get_state?
I found some other extension modules use local macro do this behavior too. such as: https://github.com/python/cpython/blob/master/Modules/_hashopenssl.c#L55

Thanks a million, victor. You helpe me merge much PRs today ;)

A static inline function is a bit better than a macro.

Sure, I remember victor have explained in other bpo/PR(oh, I forgot which one is). It's easy for debuging and something else.
I will try to add this inline function in free time.

…into bpo_1635741_9

Looks CI gate of macOS have broken, I saw some other PR have this problem.

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I remove assert operation in get_locale_state(), becasue this assert operation always failed when I add traverse slot.
I am not check the code detail. From this result, removing the assert() from get_locale_state() is ok, because we have chance get A NULL state.

some error info like:

$ ./python -E -S -m sysconfig --generate-posix-vars
python: ./Modules/_localemodule.c:52: get_locale_state: Assertion `state != ((void *)0)' failed.
[1]    17894 abort      ./python -E -S -m sysconfig --generate-posix-vars

and the trace back:

#0  0x00007ffff6ce2207 in raise () from /lib64/libc.so.6
#1  0x00007ffff6ce38f8 in abort () from /lib64/libc.so.6
#2  0x00007ffff6cdb026 in __assert_fail_base () from /lib64/libc.so.6
#3  0x00007ffff6cdb0d2 in __assert_fail () from /lib64/libc.so.6
#4  0x00000000005986f3 in get_locale_state (m=<optimized out>) at ./Modules/_localemodule.c:52
#5  locale_traverse (m=<optimized out>, visit=0x462d26 <bad_traverse_test>, arg=0x0) at ./Modules/_localemodule.c:780
#6  0x000000000046384c in PyModule_FromDefAndSpec2TraceRefs (def=def@entry=0x96e680 <_localemodule>, spec=spec@entry=0x7ffff00567d0, module_api_version=module_api_version@entry=1013)
    at Objects/moduleobject.c:370
#7  0x0000000000512153 in _imp_create_builtin (module=<optimized out>, spec=0x7ffff00567d0) at Python/import.c:1300

I have made the requested changes; please review again.

Thanks for making the requested changes!

@encukou: please review the changes made to this pull request.

The assertion was good! If you remove it, you should add if (state != NULL) checks after any use of get_locale_state.
It's actually only the traverse/clear/free functions that can get a NULL module state. They can use PyModule_GetState with a NULL check directly.

I hope you don't mind me pushing to this branch directly – I already made the change and ran tests on it locally.

The assertion was good! If you remove it, you should add if (state != NULL) checks after any use of get_locale_state.
It's actually only the traverse/clear/free functions that can get a NULL module state. They can use PyModule_GetState with a NULL check directly.

I hope you don't mind me pushing to this branch directly – I already made the change and ran tests on it locally.

petr, pls go ahead ;)

The assertion was good! If you remove it, you should add if (state != NULL) checks after any use of get_locale_state.
It's actually only the traverse/clear/free functions that can get a NULL module state. They can use PyModule_GetState with a NULL check directly.

I hope you don't mind me pushing to this branch directly – I already made the change and ran tests on it locally.

Hi, petr. Do you have wins env to test this PR? #18608.
This' PR's failed info is weird. (Looks like module state have been confused~)

Looks good to me. I plan to merge when I have time to watch the buildbots afterwards.

As I asked on your two other PRs (binascii, audioop), I don't think that state can be NULL here. Same remark in locale_clear().

Hi, victor. There have some description info about traverse slot in https://docs.python.org/3/c-api/module.html?highlight=pymoduledef#c.PyModuleDef:

A traversal function to call during GC traversal of the module object, or NULL if not needed. This function may be called before module state is allocated (PyModule_GetState() may return NULL), and before the Py_mod_exec function is executed.

So this behavior is a planned behavior, isn't it?

Let's discuss that in https://bugs.python.org/issue39824

The discussion won't be over before Nick Coghlan is satisfied (and that's a good thing!)

Meanwhile, this PR is correct according to current behavior and documentation. I don't think bpo-39824 should block it.

Alright. I expected https://bugs.python.org/issue39824 to be resolved quick, but I'm ok to merge @shihai1991 PR's first, and revisit the code later if we decided that m_clear/m_free cannot be called with a NULL state.

Port _locale extension module to multiphase initialization (PEP 489)

4ce0c7c

the-knights-who-say-ni added the CLA signed label Feb 5, 2020

bedevere-bot added the awaiting review label Feb 5, 2020

📜🤖 Added by blurb_it.

c0e0174

shihai1991 requested review from encukou and ncoghlan February 5, 2020 07:57

shihai1991 requested a review from vstinner February 17, 2020 12:32

vstinner reviewed Feb 17, 2020

View reviewed changes

shihai1991 added 2 commits February 17, 2020 23:05

add get_locale_state()

3aaf488

Merge branch 'bpo_1635741_9' of https://github.com/shihai1991/cpython …

8e0e2bf

…into bpo_1635741_9

vstinner reviewed Feb 17, 2020

View reviewed changes

Comment thread Modules/_localemodule.c Outdated

vstinner reviewed Feb 17, 2020

View reviewed changes

Comment thread Modules/_localemodule.c Outdated

fix format bug

2a4ac1b

encukou requested changes Feb 18, 2020

View reviewed changes

Comment thread Modules/_localemodule.c

bedevere-bot removed the awaiting review label Feb 18, 2020

bedevere-bot added the awaiting changes label Feb 18, 2020

locale extension module add clean slots

70fddf4

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 18, 2020

bedevere-bot requested a review from encukou February 18, 2020 15:38

Add back assertion in get_locale_state

f3a1d08

encukou approved these changes Feb 25, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 25, 2020

vstinner reviewed Feb 29, 2020

View reviewed changes

vstinner merged commit a158168 into python:master Mar 11, 2020

bedevere-bot removed the awaiting merge label Mar 11, 2020

kylotan mannequin mentioned this pull request Sep 19, 2022

Py_Finalize() doesn't clear all Python objects at exit #44470

Closed

+                      state = PyModule_GetState(self);
+                      if (state == NULL) {
+                          return NULL;
+                      }

+              locale_traverse(PyObject *m, visitproc visit, void *arg)
+              {
+                  _locale_state *state = (_locale_state*)PyModule_GetState(m);
+                  if (state) {

bpo-1635741: Port _locale extension module to multiphase initialization (PEP 489) by shihai1991 · Pull Request #18358 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-1635741: Port _locale extension module to multiphase initialization (PEP 489) - #18358

bpo-1635741: Port _locale extension module to multiphase initialization (PEP 489)#18358vstinner merged 7 commits intopython:masterpython/cpython:masterfrom shihai1991:bpo_1635741_9shihai1991/cpython:bpo_1635741_9Copy head branch name to clipboard

Conversation

shihai1991 commented Feb 5, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shihai1991 commented Feb 18, 2020

Uh oh!

Uh oh!

bedevere-bot commented Feb 18, 2020

Uh oh!

shihai1991 commented Feb 18, 2020

Uh oh!

shihai1991 commented Feb 18, 2020

Uh oh!

bedevere-bot commented Feb 18, 2020

Uh oh!

encukou commented Feb 25, 2020

Uh oh!

shihai1991 commented Feb 25, 2020

Uh oh!

shihai1991 commented Feb 25, 2020

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bpo-1635741: Port _locale extension module to multiphase initialization (PEP 489)#18358
vstinner merged 7 commits into
python:masterfrom
shihai1991:bpo_1635741_9

shihai1991 commented Feb 5, 2020 •
edited by bedevere-bot
Loading

codecov Bot commented Feb 5, 2020 •
edited
Loading