gh-116437: Use new C API PyDict_Pop() to simplify the code by serhiy-storchaka · Pull Request #116438 · python/cpython

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.

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

- 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

+ 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

 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

 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

Comment threadModules/_csv.c Outdated
Comment threadModules/_threadmodule.c Outdated
Comment threadObjects/moduleobject.c Outdated
Comment threadObjects/moduleobject.c Outdated
Comment threadObjects/typeobject.c
Comment threadObjects/typeobject.c

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!

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