gh-114670: Fix `_testbuffer` module initialization #114672

sobolevn · 2024-01-28T07:57:21Z

@erlend-aasland would you please take a look? :)
I think you've already reviewed several similar PRs from me.

The change is straight-forward: check for errors, clean things up.

Issue: _testbuffer.c's initialization does not handle errors #114670

Modules/_testbuffer.c

serhiy-storchaka

PyImport_ImportModule() returns a new reference. structmodule is a leaked reference.

sobolevn · 2024-02-07T12:15:03Z

simple_format = PyUnicode_FromString(simple_fmt); was also leaking.

serhiy-storchaka

Ah, wait. structmodule and simple_format are global variables. They should hold a reference.

Previous version was more correct, I am sorry.

This reverts commit c36c054.

sobolevn · 2024-02-07T13:39:52Z

@serhiy-storchaka when this PR will be ready, can you please let me merge it? :)

serhiy-storchaka · 2024-02-07T14:12:39Z

Of course, it is now your work.

serhiy-storchaka · 2024-02-07T14:14:18Z

Just don't forget to clean up the commit message before merging.

Modules/_testbuffer.c

Misc/NEWS.d/next/Core and Builtins/2024-01-28-22-32-07.gh-issue-114672.9iEHAg.rst

Modules/_testbuffer.c

vstinner · 2024-02-08T17:45:26Z

Modules/_testbuffer.c

    Py_SET_TYPE(&NDArray_Type, &PyType_Type);
-    Py_INCREF(&NDArray_Type);
-    PyModule_AddObject(m, "ndarray", (PyObject *)&NDArray_Type);
+    if (PyModule_AddObjectRef(m, "ndarray", (PyObject *)&NDArray_Type) < 0) {


You might use PyModule_AddType().

Looks like it the best one here 👍

Modules/_testbuffer.c

Misc/NEWS.d/next/Core and Builtins/2024-01-28-22-32-07.gh-issue-114672.9iEHAg.rst

serhiy-storchaka

I would remove the NEWS entry at all.

There are no leaks during _testbuffer initialization in normal case. They are only occurred if the initialization fails.
But the module still leaks references when it is reloaded. It is not normal case, and it does not support reloading.
It is an internal module only used in internal tests. Common Python users never use it. The NEWS entry is purposed for Python users.

So it is rather an internal refactoring and an intermediate step to make _testbuffer reloadable, but it is not practical to make the next step.

…e-114672.9iEHAg.rst

serhiy-storchaka

LGTM.

vstinner

LGTM.

@sobolevn: Maybe it's a first good PR to merge for your new role 😇

sobolevn · 2024-02-10T21:48:55Z

Thanks a lot, everyone! 👍
Especially @vstinner for guiding me through the merge process :)

bedevere-bot · 2024-02-11T01:02:47Z

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

Hi! The buildbot AMD64 Windows11 Bigmem 3.x has failed when building commit 3a5b38e.

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/1079/builds/3360) and take a look at the build logs.
Check if the failure is related to this commit (3a5b38e) 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/1079/builds/3360

Failed tests:

test_queue

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

==

Click to see traceback logs

remote: Enumerating objects: 9, done.        
remote: Counting objects:  11% (1/9)        
remote: Counting objects:  22% (2/9)        
remote: Counting objects:  33% (3/9)        
remote: Counting objects:  44% (4/9)        
remote: Counting objects:  55% (5/9)        
remote: Counting objects:  66% (6/9)        
remote: Counting objects:  77% (7/9)        
remote: Counting objects:  88% (8/9)        
remote: Counting objects: 100% (9/9)        
remote: Counting objects: 100% (9/9), done.        
remote: Compressing objects:  14% (1/7)        
remote: Compressing objects:  28% (2/7)        
remote: Compressing objects:  42% (3/7)        
remote: Compressing objects:  57% (4/7)        
remote: Compressing objects:  71% (5/7)        
remote: Compressing objects:  85% (6/7)        
remote: Compressing objects: 100% (7/7)        
remote: Compressing objects: 100% (7/7), done.        
remote: Total 9 (delta 2), reused 3 (delta 2), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '3a5b38e3b465e00f133ff8074a2d4afb1392dfb5'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 3a5b38e3b4 gh-114670: Fix `_testbuffer` module initialization (#114672)
Switched to and reset branch 'main'

Could Not Find R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\*.pyc
The system cannot find the file specified.
Could Not Find R:\buildarea\3.x.ambv-bb-win11.bigmem\build\PCbuild\python*.zip

Could Not Find R:\buildarea\3.x.ambv-bb-win11.bigmem\build\PCbuild\python*.zip

sobolevn · 2024-02-11T07:34:06Z

The failure is unrelated and kwown, it happened before: https://github.com/python/cpython/actions/runs/7855933464/job/21438170525
Reported here: #115258

sobolevn · 2024-02-11T07:35:21Z

Since https://github.com/python/cpython/blob/3.11/Modules/_testbuffer.c exists, I will make backports.

miss-islington-app · 2024-02-11T07:35:31Z

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington-app · 2024-02-11T07:35:31Z

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

…4672) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

bedevere-app · 2024-02-11T07:35:41Z

GH-115271 is a backport of this pull request to the 3.11 branch.

…4672) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

bedevere-app · 2024-02-11T07:35:45Z

GH-115272 is a backport of this pull request to the 3.12 branch.

…#115272) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

…#115271) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

pythongh-114670: Fix _testbuffer module initialization

7d9f2fd

sobolevn requested a review from erlend-aasland January 28, 2024 07:57

bedevere-app bot added the awaiting review label Jan 28, 2024

bedevere-app bot mentioned this pull request Jan 28, 2024

_testbuffer.c's initialization does not handle errors #114670

Closed

bedevere-app bot added the tests Tests in the Lib/test dir label Jan 28, 2024

serhiy-storchaka reviewed Jan 28, 2024

View reviewed changes

Modules/_testbuffer.c Outdated Show resolved Hide resolved

Modules/_testbuffer.c Outdated Show resolved Hide resolved

Address review

63c2bb2

sobolevn requested a review from serhiy-storchaka February 7, 2024 07:23

serhiy-storchaka reviewed Feb 7, 2024

View reviewed changes

sobolevn added 2 commits February 7, 2024 14:17

Merge branch 'main' into issue-114670

9e5e81a

Do not leak structmodule and simple_format

c36c054

serhiy-storchaka reviewed Feb 7, 2024

View reviewed changes

Revert "Do not leak structmodule and simple_format"

5a56dbe

This reverts commit c36c054.

serhiy-storchaka approved these changes Feb 7, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Feb 7, 2024

vstinner reviewed Feb 7, 2024

View reviewed changes

Modules/_testbuffer.c Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Core and Builtins/2024-01-28-22-32-07.gh-issue-114672.9iEHAg.rst Outdated Show resolved Hide resolved

Address review

b8ecc30

vstinner reviewed Feb 8, 2024

View reviewed changes

Address review

cfcfb6a

serhiy-storchaka reviewed Feb 8, 2024

View reviewed changes

Delete Misc/NEWS.d/next/Core and Builtins/2024-01-28-22-32-07.gh-issu…

3d95285

…e-114672.9iEHAg.rst

serhiy-storchaka approved these changes Feb 8, 2024

View reviewed changes

sobolevn added the skip news label Feb 8, 2024

vstinner approved these changes Feb 9, 2024

View reviewed changes

sobolevn merged commit 3a5b38e into python:main Feb 10, 2024

bedevere-app bot removed the awaiting merge label Feb 10, 2024

sobolevn deleted the issue-114670 branch February 10, 2024 21:48

sobolevn added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Feb 11, 2024

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 11, 2024

pythongh-114670: Fix _testbuffer module initialization (pythonGH-11…

a16ee49

…4672) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 11, 2024

pythongh-114670: Fix _testbuffer module initialization (pythonGH-11…

086fcf1

…4672) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 11, 2024

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Feb 11, 2024

sobolevn added a commit that referenced this pull request Feb 11, 2024

[3.12] gh-114670: Fix _testbuffer module initialization (GH-114672) (…

d19f067

…#115272) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

sobolevn added a commit that referenced this pull request Feb 11, 2024

[3.11] gh-114670: Fix _testbuffer module initialization (GH-114672) (…

f4146a2

…#115271) (cherry picked from commit 3a5b38e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024

pythongh-114670: Fix _testbuffer module initialization (python#114672)

0d97aec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-114670: Fix `_testbuffer` module initialization #114672

gh-114670: Fix `_testbuffer` module initialization #114672

sobolevn commented Jan 28, 2024 •

edited by bedevere-app bot

Loading

serhiy-storchaka left a comment

sobolevn commented Feb 7, 2024

serhiy-storchaka left a comment

sobolevn commented Feb 7, 2024

serhiy-storchaka commented Feb 7, 2024

serhiy-storchaka commented Feb 7, 2024

vstinner Feb 8, 2024

sobolevn Feb 8, 2024

serhiy-storchaka left a comment

serhiy-storchaka left a comment

vstinner left a comment

sobolevn commented Feb 10, 2024

bedevere-bot commented Feb 11, 2024

sobolevn commented Feb 11, 2024

sobolevn commented Feb 11, 2024

miss-islington-app bot commented Feb 11, 2024

miss-islington-app bot commented Feb 11, 2024

bedevere-app bot commented Feb 11, 2024

bedevere-app bot commented Feb 11, 2024

gh-114670: Fix _testbuffer module initialization #114672

gh-114670: Fix _testbuffer module initialization #114672

Conversation

sobolevn commented Jan 28, 2024 • edited by bedevere-app bot Loading

serhiy-storchaka left a comment

Choose a reason for hiding this comment

sobolevn commented Feb 7, 2024

serhiy-storchaka left a comment

Choose a reason for hiding this comment

sobolevn commented Feb 7, 2024

serhiy-storchaka commented Feb 7, 2024

serhiy-storchaka commented Feb 7, 2024

vstinner Feb 8, 2024

Choose a reason for hiding this comment

sobolevn Feb 8, 2024

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

sobolevn commented Feb 10, 2024

bedevere-bot commented Feb 11, 2024

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

sobolevn commented Feb 11, 2024

sobolevn commented Feb 11, 2024

miss-islington-app bot commented Feb 11, 2024

miss-islington-app bot commented Feb 11, 2024

bedevere-app bot commented Feb 11, 2024

bedevere-app bot commented Feb 11, 2024

gh-114670: Fix `_testbuffer` module initialization #114672

gh-114670: Fix `_testbuffer` module initialization #114672

sobolevn commented Jan 28, 2024 •

edited by bedevere-app bot

Loading