Skip to content

Conversation

wiomoc
Copy link
Contributor

@wiomoc wiomoc commented Aug 25, 2025

@wiomoc wiomoc force-pushed the gh-135763-clinic_allow_negative branch 3 times, most recently from 42f8e13 to 4ff6d71 Compare August 25, 2025 23:58
@AA-Turner AA-Turner self-requested a review August 26, 2025 01:04
@wiomoc wiomoc force-pushed the gh-135763-clinic_allow_negative branch 2 times, most recently from 8d977a3 to a36d270 Compare August 26, 2025 09:07
@wiomoc wiomoc marked this pull request as ready for review August 26, 2025 09:08
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Please revert all changes that are not tests. Do not use it yet. In addition, consider expanding the tests in test_clinic.py if possible.

@bedevere-app
Copy link

bedevere-app bot commented Aug 26, 2025

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.

@wiomoc wiomoc force-pushed the gh-135763-clinic_allow_negative branch from a36d270 to 119ccf3 Compare August 26, 2025 15:15
@wiomoc wiomoc force-pushed the gh-135763-clinic_allow_negative branch from 119ccf3 to 31863c4 Compare August 26, 2025 15:18
@wiomoc
Copy link
Contributor Author

wiomoc commented Aug 26, 2025

I have made the requested changes; please review again

I am going to raise a follow-up issue for migrating the existing manual bound checks, after this PR landed

@bedevere-app
Copy link

bedevere-app bot commented Aug 26, 2025

Thanks for making the requested changes!

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

@bedevere-app bedevere-app bot requested a review from picnixz August 26, 2025 15:25
@@ -0,0 +1,2 @@
Add ``allow_negative`` flag to ``Py_ssize_t`` Argument Clinic converter, to
Copy link
Member

Choose a reason for hiding this comment

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

AC is unfortunately an internal tool so I don't think we should document it. However, what could be interesting is whether we want to expose a new parsing character for positive Py_ssize_t values. For now, https://docs.python.org/3/c-api/arg.html#numbers only documents n for Py_ssize_t but maybe having something for Py_ssize_t >= 0 would make sense (I don't know, so for now, don't change anything).

Copy link
Contributor Author

@wiomoc wiomoc Aug 31, 2025

Choose a reason for hiding this comment

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

AC is unfortunately an internal tool so I don't think we should document it.

There are already entries regarding arg clinic in the changelog 1, therefore I think it's plausible to keep the entry. I moved it to tools/demos however - should be a better fit.

Whether we want to expose a new parsing character for positive Py_ssize_t values.

I wonder why not providing a format unit for conversion to size_t directly.
The only advantage is see with Py_ssize_t(allow_negative) in comparison to size_t is the fact that you could provide a negative value e.g. -1 default value as undefined / None sentinel.

Copy link
Member

Choose a reason for hiding this comment

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

I agree no NEWS entry is needed, but @wiomoc could create a PR for the devguide after this is merged, which is where we document AC.

Copy link
Member

@picnixz picnixz Aug 31, 2025

Choose a reason for hiding this comment

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

I wonder why not providing a format unit for conversion to size_t directly.

Because we use ssize_t for most internal sizes in Python as we need to indicate when a conversion failed with value = -1 && PyErr_Occurred() (I think).

The only advantage is see with Py_ssize_t(allow_negative) in comparison to size_t is the fact that you could provide a negative value e.g. -1 default value as undefined / None sentinel.

Unfortunately, internally, most structures use ssize_t for sizes and not size_t, so we could have overflows here (that is, we pass a value that fits on a size_t but not on a ssize_t). See Py_buffer for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. Now I can make sense of it. I'll create an issue for adding N as the format unit for Py_ssize_t >= 0.

@wiomoc
Copy link
Contributor Author

wiomoc commented Aug 31, 2025

I have made the requested changes; please review again

@bedevere-app bedevere-app bot requested a review from picnixz August 31, 2025 18:10
@bedevere-app
Copy link

bedevere-app bot commented Sep 1, 2025

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.

@AA-Turner AA-Turner merged commit 47bc10e into python:main Sep 1, 2025
58 of 59 checks passed
@bedevere-bot
Copy link

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

Hi! The buildbot s390x RHEL9 Refleaks 3.x (tier-3) has failed when building commit 47bc10e.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1589/builds/2061) and take a look at the build logs.
  4. Check if the failure is related to this commit (47bc10e) or if it is a false positive.
  5. 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/#/builders/1589/builds/2061

Failed tests:

  • test.test_multiprocessing_fork.test_processes

Failed subtests:

  • test_interrupt - test.test_multiprocessing_fork.test_processes.WithProcessesTestProcess.test_interrupt

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 596, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 577, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 573, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-3' pid=281260 parent=281256 started daemon>


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/process.py", line 320, in _bootstrap
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 524, in _sleep_some_event
    time.sleep(100)
    ~~~~~~~~~~^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/libregrtest/setup.py", line 83, in _test_audit_hook
    def _test_audit_hook(name, args):
    
KeyboardInterrupt
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/process.py", line 320, in _bootstrap
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 524, in _sleep_some_event
    time.sleep(100)
    ~~~~~~~~~~^^^^^
KeyboardInterrupt
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 596, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 577, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-rhel9-s390x.refleak/build/Lib/test/_test_multiprocessing.py", line 573, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-539' pid=208855 parent=200492 started daemon>

@bedevere-bot
Copy link

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

Hi! The buildbot AMD64 Fedora Stable Refleaks 3.x (tier-1) has failed when building commit 47bc10e.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/320/builds/2966) and take a look at the build logs.
  4. Check if the failure is related to this commit (47bc10e) or if it is a false positive.
  5. 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/#/builders/320/builds/2966

Failed tests:

  • test.test_multiprocessing_fork.test_processes

Failed subtests:

  • test_interrupt - test.test_multiprocessing_fork.test_processes.WithProcessesTestProcess.test_interrupt

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 596, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 577, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 573, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-542' pid=2609465 parent=2601899 started daemon>


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 320, in _bootstrap
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 524, in _sleep_some_event
    time.sleep(100)
    ~~~~~~~~~~^^^^^
KeyboardInterrupt
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 596, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 577, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 573, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-5' pid=2718849 parent=2718843 started daemon>

lkollar pushed a commit to lkollar/cpython that referenced this pull request Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants