GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-116437: Use new C API PyDict_Pop() to simplify the code by serhiy-storchaka · Pull Request #116438 · python/cpython

Conversation

serhiy-storchaka commented Mar 6, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Issue: Use PyDict_Pop() #116437

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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-app Bot added the awaiting core review label

Mar 6, 2024

bedevere-app Bot 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

serhiy-storchaka commented Mar 6, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reviewed

Mar 6, 2024

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Modules/_csv.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_threadmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Address review comments.

9109ccb

vstinner approved these changes

Mar 7, 2024

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Mar 7, 2024

serhiy-storchaka merged commit 72d3cc9 into python:main

Mar 7, 2024

bedevere-app Bot removed the awaiting merge label

Mar 7, 2024

serhiy-storchaka deleted the use-pydict-pop branch

March 7, 2024 09:21

serhiy-storchaka commented Mar 7, 2024

Copy link

Copy Markdown

Member Author

Thank you for your review Victor.

vstinner reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot commented Mar 7, 2024

Copy link

Copy Markdown

⚠️⚠️⚠️ 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_script
    raise AssertionError(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_midnight
    self.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_inner
    self.run()
    ~~~~~~~~^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/threading.py", line 1017, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/queue.py", line 85, in task_done
    raise ValueError('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_timeout
    self.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_flock
    self._lock(fcntl.flock, "flock")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("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_lockf
    self._lock(fcntl.lockf, "lockf")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("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_midnight
    self.assertIn(f'testing2 {i}', line)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'testing2 0' not found in '2024-03-07 11:49:20,006 testing1 0\n'

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently reviewed

Mar 7, 2024

View reviewed changes

ericsnowcurrently left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/posixmodule.c

Comment on lines -17559 to +17563

		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;

ericsnowcurrently Mar 7, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Mar 7, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignoring errors is always a good thing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Parser/asdl_c.py

Comment on lines +1081 to -1093

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

ericsnowcurrently Mar 7, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch about fixing the decrefs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Python/pythonrun.c

Comment on lines 475 to 480

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

ericsnowcurrently Mar 7, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Using PyErr_Print() here makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Modules/_asynciomodule.c

Comment on lines 2045 to +2057

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

ericsnowcurrently Mar 7, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Mar 7, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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)

Reviewers

vstinner

vstinner approved these changes

ericsnowcurrently

ericsnowcurrently left review comments

isidentical

Awaiting requested review from isidentical

pganssle

Awaiting requested review from pganssle

pganssle is a code owner

abalkin

Awaiting requested review from abalkin

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

gvanrossum

Awaiting requested review from gvanrossum

1st1

Awaiting requested review from 1st1

1st1 is a code owner

asvetlov

Awaiting requested review from asvetlov

asvetlov is a code owner

kumaraditya303

Awaiting requested review from kumaraditya303

kumaraditya303 is a code owner

willingc

Awaiting requested review from willingc

willingc is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

serhiy-storchaka commented Mar 6, 2024 • edited by bedevere-app Bot Loading 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!

ericsnowcurrently Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serhiy-storchaka commented Mar 6, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.