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

shihai1991 · 2020-02-05T07:54:12Z

https://bugs.python.org/issue1635741

codecov · 2020-02-05T08:29:58Z

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.

vstinner · 2020-02-17T13:47:50Z

Modules/_localemodule.c

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


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

Modules/_localemodule.c

shihai1991 · 2020-02-18T00:36:00Z

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

Modules/_localemodule.c

bedevere-bot · 2020-02-18T11:36:18Z

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.

shihai1991 · 2020-02-18T15:12:44Z

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

shihai1991 · 2020-02-18T15:38:08Z

I have made the requested changes; please review again.

bedevere-bot · 2020-02-18T15:38:11Z

Thanks for making the requested changes!

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

encukou · 2020-02-25T15:11:13Z

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.

shihai1991 · 2020-02-25T15:15:59Z

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 ;)

shihai1991 · 2020-02-25T15:21:28Z

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~)

encukou

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

vstinner · 2020-02-29T23:39:19Z

Modules/_localemodule.c

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


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

Modules/_localemodule.c Outdated Show resolved Hide resolved

vstinner reviewed Feb 17, 2020

View reviewed changes

Modules/_localemodule.c Outdated Show resolved Hide resolved

fix format bug

2a4ac1b

encukou requested changes Feb 18, 2020

View reviewed changes

Modules/_localemodule.c Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shihai1991 commented Feb 5, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 5, 2020 •

edited

Loading

vstinner Feb 17, 2020

shihai1991 Feb 17, 2020

encukou Feb 18, 2020

shihai1991 Feb 18, 2020

shihai1991 commented Feb 18, 2020

bedevere-bot commented Feb 18, 2020

shihai1991 commented Feb 18, 2020

shihai1991 commented Feb 18, 2020

bedevere-bot commented Feb 18, 2020

encukou commented Feb 25, 2020

shihai1991 commented Feb 25, 2020

shihai1991 commented Feb 25, 2020

encukou left a comment

vstinner Feb 29, 2020

shihai1991 Mar 1, 2020

vstinner Mar 2, 2020

encukou Mar 11, 2020

vstinner Mar 11, 2020

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

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

Conversation

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

codecov bot commented Feb 5, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihai1991 commented Feb 18, 2020

bedevere-bot commented Feb 18, 2020

shihai1991 commented Feb 18, 2020

shihai1991 commented Feb 18, 2020

bedevere-bot commented Feb 18, 2020

encukou commented Feb 25, 2020

shihai1991 commented Feb 25, 2020

shihai1991 commented Feb 25, 2020

encukou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihai1991 commented Feb 5, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 5, 2020 •

edited

Loading