gh-116437: Use new C API PyDict_Pop() to simplify the code#116438

serhiy-storchaka · 2024-03-06T21:36:23Z

Issue: Use PyDict_Pop() #116437

serhiy-storchaka · 2024-03-06T22:04:29Z

It also fixes leaks in Python/Python-ast.c.

vstinner

Maybe replace == -1 with < 0 to check for failure.

Modules/_csv.c

Modules/_threadmodule.c

Objects/moduleobject.c

Objects/typeobject.c

vstinner

LGTM.

I don't know if < 0 to check for errors is a common idiom, but I prefer to use it rather than == 0. Currently, https://devguide.python.org/developer-workflow/c-api/index.html#guidelines-for-expanding-changing-the-public-api only mentions "return -1: internal error or API misuse; exception raised".

serhiy-storchaka · 2024-03-07T09:21:45Z

Thank you for your review Victor.

bedevere-bot · 2024-03-07T12:19:56Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL Refleaks 3.x has failed when building commit 72d3cc9.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1226/builds/1401) and take a look at the build logs.
Check if the failure is related to this commit (72d3cc9) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1226/builds/1401

Failed tests:

test_logging
test_eintr

Failed subtests:

test_flock - main.FNTLEINTRTest.test_flock
test_all - test.test_eintr.EINTRTests.test_all
test_rollover_at_midnight - test.test_logging.TimedRotatingFileHandlerTest.test_rollover_at_midnight
test_map_timeout - test.test_concurrent_futures.test_process_pool.ProcessPoolSpawnProcessPoolExecutorTest.test_map_timeout
test_lockf - main.FNTLEINTRTest.test_lockf

Test leaking resources:

test_queue: memory blocks

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last): File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/test_eintr.py", line 17, in test_all script_helper.run_test_script(script) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^ File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/support/script_helper.py", line 316, in run_test_scriptraiseAssertionError(f"{name} failed") AssertionError: script _test_eintr.py failed Traceback (most recent call last): File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/test_logging.py", line 6111, in test_rollover_at_midnightself.assertIn(f'testing2 {i}', line) ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^AssertionError: 'testing2 0' not found in '2024-03-07 12:13:26,043 testing1 0\n' Traceback (most recent call last): File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/threading.py", line 1080, in _bootstrap_innerself.run() ~~~~~~~~^^ File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/threading.py", line 1017, in runself._target(*self._args, **self._kwargs) ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/queue.py", line 85, in task_doneraiseValueError('task_done() called too many times') ValueError: task_done() called too many times k Traceback (most recent call last): File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/test_concurrent_futures/executor.py", line 71, in test_map_timeoutself.assertEqual([None, None], results) ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^AssertionError: Lists differ: [None, None] != [] Traceback (most recent call last): File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/_test_eintr.py", line 535, in test_flockself._lock(fcntl.flock, "flock") ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^ File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/_test_eintr.py", line 517, in _lockraiseException("failed to sync child in %.1f sec"% dt) Exception: failed to sync child in 300.5 sec Traceback (most recent call last): File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/_test_eintr.py", line 532, in test_lockfself._lock(fcntl.lockf, "lockf") ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^ File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/_test_eintr.py", line 517, in _lockraiseException("failed to sync child in %.1f sec"% dt) Exception: failed to sync child in 300.6 sec Traceback (most recent call last): File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/test_logging.py", line 6111, in test_rollover_at_midnightself.assertIn(f'testing2 {i}', line) ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^AssertionError: 'testing2 0' not found in '2024-03-07 11:49:20,006 testing1 0\n'

ericsnowcurrently

Sorry for the post-merge review. I was curious about what changed and happened to notice a few things.

ericsnowcurrently · 2024-03-07T16:48:59Z

Modules/posixmodule.c

- if (PyDict_DelItemString(dct, "pwritev") == -1){
-PyErr_Clear();
+ if (PyDict_PopString(dct, "pwritev", NULL) < 0){
+return -1;
 }
- if (PyDict_DelItemString(dct, "preadv") == -1){
-PyErr_Clear();
+ if (PyDict_PopString(dct, "preadv", NULL) < 0){
+return -1;


This is slightly different behavior, isn't it? Before, we tried removing each item and ignored all errors. Now we fail at the first error. Is that okay? (Maybe @ronaldoussoren has some thoughts on this?) Was this intentional?
(The same applies to the changes in Modules/timemodule.c.)

Not ignoring errors is always a good thing.

ericsnowcurrently · 2024-03-07T16:58:11Z

Parser/asdl_c.py

+ int rc = PyDict_Pop(remaining_dict, name, &value);
+ Py_DECREF(name);
+ if (rc < 0){
+ goto cleanup;
+ }
 if (!value){
- if (PyErr_Occurred()){
- goto cleanup;
- }
 break;
 }
- if (PyList_Append(positional_args, value) < 0){
+ rc = PyList_Append(positional_args, value);
+ Py_DECREF(value);
+ if (rc < 0){
 goto cleanup;
 }
- if (PyDict_DelItem(remaining_dict, name) < 0){
- goto cleanup;
- }
- Py_DECREF(name);


Good catch about fixing the decrefs.

ericsnowcurrently · 2024-03-07T17:04:00Z

Python/pythonrun.c

 if (set_file_name){
- if (PyDict_DelItemString(dict, "__file__")){
-PyErr_Clear();
+ if (PyDict_PopString(dict, "__file__", NULL) < 0){
+PyErr_Print();
 }
- if (PyDict_DelItemString(dict, "__cached__")){
-PyErr_Clear();
+ if (PyDict_PopString(dict, "__cached__", NULL) < 0){
+PyErr_Print();
 }


Nice. Using PyErr_Print() here makes sense.

ericsnowcurrently · 2024-03-07T17:10:56Z

Modules/_asynciomodule.c

 swap_current_task(asyncio_state *state, PyObject *loop, PyObject *task)
 {
 PyObject *prev_task;
+
+ if (task == Py_None){
+ if (PyDict_Pop(state->current_tasks, loop, &prev_task) < 0){
+ return NULL;
+ }
+ if (prev_task == NULL){
+ Py_RETURN_NONE;
+ }
+ return prev_task;
+ }


It took me a minute to make sense of your changes in this file, but now I get it. 👍

Same here. I blocked here for 5 minutes :-D

…honGH-116438)

pythongh-116437: Use new C API PyDict_Pop() to simplify the code
b933bf8

serhiy-storchaka added the skip news label Mar 6, 2024

serhiy-storchaka requested a review from vstinner March 6, 2024 21:36

serhiy-storchaka requested review from 1st1, abalkin, asvetlov, ericsnowcurrently, gvanrossum, isidentical, kumaraditya303, markshannon, pganssle and willingc as code owners March 6, 2024 21:36

bedevere-appbot added the awaiting core review label Mar 6, 2024

bedevere-appbot mentioned this pull request Mar 6, 2024
Use PyDict_Pop() #116437
Closed

Fix a leak in _localdummy_destroyed.
989fd4f

serhiy-storchaka mentioned this pull request Mar 6, 2024
gh-105858: Improve AST node constructors #105880
Merged

vstinner reviewed Mar 6, 2024
View reviewed changes

Address review comments.
9109ccb

vstinner approved these changes Mar 7, 2024
View reviewed changes

bedevere-appbot added awaiting merge and removed awaiting core review labels Mar 7, 2024

serhiy-storchaka merged commit 72d3cc9 into python:mainMar 7, 2024

bedevere-appbot removed the awaiting merge label Mar 7, 2024

serhiy-storchaka deleted the use-pydict-pop branch March 7, 2024 09:21

ericsnowcurrently reviewed Mar 7, 2024
View reviewed changes

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
pythongh-116437: Use new C API PyDict_Pop() to simplify the code (pyt…
33c372d
…honGH-116438)

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
pythongh-116437: Use new C API PyDict_Pop() to simplify the code (pyt…
9e1cf2f
…honGH-116438)

Uh oh!

gh-116437: Use new C API PyDict_Pop() to simplify the code#116438

gh-116437: Use new C API PyDict_Pop() to simplify the code #116438

Uh oh!

Conversation

serhiy-storchaka commented Mar 6, 2024• edited by bedevere-app botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Mar 6, 2024

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Mar 7, 2024

Uh oh!

bedevere-bot commented Mar 7, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrentlyMar 7, 2024

Choose a reason for hiding this comment

Uh oh!

vstinnerMar 7, 2024

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrentlyMar 7, 2024

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrentlyMar 7, 2024

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrentlyMar 7, 2024

Choose a reason for hiding this comment

Uh oh!

vstinnerMar 7, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serhiy-storchaka commented Mar 6, 2024•
edited by bedevere-app bot
Loading