gh-121474: Add threading.Barrier parties arg sanity check. #121480

pygeek · 2024-07-08T07:33:09Z

Issue: Barrier hangs indefinitely when arg parties < 0 or invalid type #121474

graingert · 2024-07-08T09:50:47Z

This matches the asyncio.Barrier now, which is great. But I notice neither handle 1.5 and other non-integral floats well

pygeek · 2024-07-08T11:35:29Z

This matches the asyncio.Barrier now, which is great. But I notice neither handle 1.5 and other non-integral floats well

I noticed this too, but neither do any of the primitives in threading. My primary intention was to get Barrier in line with the other primitives.

I plan on creating a separate Discuss thread on the matter, once/if this current PR is merged.

My guess is that since the float behavior among primitives hasn't been a noted problem until now, that it wouldn't make sense to make the sweeping changes necessary to fix them at this point.

ZeroIntensity

Looks good, other than a nitpick and PEP 8 compliance 🙂

ZeroIntensity · 2024-07-10T17:44:56Z

Lib/test/lock_tests.py

@@ -1013,6 +1013,10 @@ def multipass(self, results, n):
        self.assertEqual(self.barrier.n_waiting, 0)
        self.assertFalse(self.barrier.broken)

+    def test_constructor(self):
+        self.assertRaises(ValueError, self.barriertype, parties = 0)
+        self.assertRaises(ValueError, self.barriertype, parties = -1)


Suggested change

self.assertRaises(ValueError, self.barriertype, parties = -1)

self.assertRaises(ValueError, self.barriertype, parties=-1)

ZeroIntensity · 2024-07-10T17:45:23Z

Lib/test/lock_tests.py

@@ -1013,6 +1013,10 @@ def multipass(self, results, n):
        self.assertEqual(self.barrier.n_waiting, 0)
        self.assertFalse(self.barrier.broken)

+    def test_constructor(self):
+        self.assertRaises(ValueError, self.barriertype, parties = 0)


Suggested change

self.assertRaises(ValueError, self.barriertype, parties = 0)

self.assertRaises(ValueError, self.barriertype, parties=0)

Lib/threading.py

pygeek · 2024-07-10T18:25:18Z

Looks good, other than a nitpick and PEP 8 compliance 🙂

Nice catch. I don't think the linter runs on test files. I literally copied the check from https://github.com/python/cpython/pull/121480/files/58880e2d5387680385e3802d8e18d2d2de8ef470#diff-1d04f1b4512c48ecc14e3eae5713be5dea3bdbeba07f9d77bb08089103c880dbL754-L755 .

I'm not sure if the omission of the linter is intentional.
Does it make sense to have a second lint check in CI that just checks Lib/tests?

Edit:

I double checked. There is a linter on Lib/tests, but ruff format --check isn't being used.

ZeroIntensity · 2024-07-10T18:27:09Z

Does it make sense to have a second lint check in CI that just checks Lib/tests?

Probably not, and out of the scope of this issue anyways.

pygeek · 2024-07-10T21:16:36Z

Does it make sense to have a second lint check in CI that just checks Lib/tests?

Probably not, and out of the scope of this issue anyways.

I was speaking more generally, outside the context of this PR. Anyway, I'll start a separate thread in Discuss on the topic.

ZeroIntensity

LGTM!

serhiy-storchaka

LGTM.

miss-islington-app · 2024-07-30T08:53:33Z

Thanks @pygeek for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington-app · 2024-07-30T08:53:33Z

Thanks @pygeek for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

…honGH-121480) (cherry picked from commit d27a53f) Co-authored-by: Clinton <pygeek@users.noreply.github.com>

bedevere-app · 2024-07-30T08:53:42Z

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

bedevere-app · 2024-07-30T08:53:47Z

GH-122444 is a backport of this pull request to the 3.13 branch.

serhiy-storchaka · 2024-07-30T08:54:19Z

Thank you for your contribution @pygeek.

…-121480) (GH-122443) (cherry picked from commit d27a53f) Co-authored-by: Clinton <pygeek@users.noreply.github.com>

…-121480) (GH-122444) (cherry picked from commit d27a53f) Co-authored-by: Clinton <pygeek@users.noreply.github.com>

…honGH-121480)

bedevere-app bot mentioned this pull request Jul 8, 2024

Barrier hangs indefinitely when arg parties < 0 or invalid type #121474

Closed

bedevere-app bot added the awaiting review label Jul 8, 2024

pygeek changed the title ~~gh-121474: Added Barrier parties sanity chack.~~ gh-121474: Add Barrier parties sanity chack. Jul 8, 2024

pygeek force-pushed the fix-barrier branch from 546dbe6 to 6b38cab Compare July 8, 2024 07:52

pygeek changed the title ~~gh-121474: Add Barrier parties sanity chack.~~ gh-121474: Add threading.Barrier parties arg sanity check. Jul 8, 2024

pygeek force-pushed the fix-barrier branch 2 times, most recently from 6878079 to 58880e2 Compare July 8, 2024 08:47

ZeroIntensity requested changes Jul 10, 2024

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Jul 10, 2024

pythongh-121474: Add threading.Barrier parties arg sanity check.

644d89b

pygeek force-pushed the fix-barrier branch from 58880e2 to 644d89b Compare July 10, 2024 18:26

ZeroIntensity approved these changes Jul 10, 2024

View reviewed changes

serhiy-storchaka approved these changes Jul 30, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Jul 30, 2024

serhiy-storchaka merged commit d27a53f into python:main Jul 30, 2024
35 checks passed

bedevere-app bot removed the awaiting merge label Jul 30, 2024

serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jul 30, 2024

bedevere-app bot removed the needs backport to 3.12 only security fixes label Jul 30, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 30, 2024

serhiy-storchaka pushed a commit that referenced this pull request Jul 30, 2024

[3.12] gh-121474: Add threading.Barrier parties arg sanity check. (GH…

ca22750

…-121480) (GH-122443) (cherry picked from commit d27a53f) Co-authored-by: Clinton <pygeek@users.noreply.github.com>

serhiy-storchaka pushed a commit that referenced this pull request Jul 30, 2024

[3.13] gh-121474: Add threading.Barrier parties arg sanity check. (GH…

202cfbf

…-121480) (GH-122444) (cherry picked from commit d27a53f) Co-authored-by: Clinton <pygeek@users.noreply.github.com>

pygeek deleted the fix-barrier branch July 30, 2024 14:50

blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024

pythongh-121474: Add threading.Barrier parties arg sanity check. (pyt…

9cf446b

…honGH-121480)

	self.assertRaises(ValueError, self.barriertype, parties = -1)
	self.assertRaises(ValueError, self.barriertype, parties=-1)

Uh oh!

gh-121474: Add threading.Barrier parties arg sanity check. #121480

gh-121474: Add threading.Barrier parties arg sanity check. #121480

Uh oh!

Conversation

pygeek commented Jul 8, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graingert commented Jul 8, 2024

Uh oh!

pygeek commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pygeek commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity commented Jul 10, 2024

Uh oh!

pygeek commented Jul 10, 2024

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app bot commented Jul 30, 2024

Uh oh!

miss-islington-app bot commented Jul 30, 2024

Uh oh!

bedevere-app bot commented Jul 30, 2024

Uh oh!

bedevere-app bot commented Jul 30, 2024

Uh oh!

serhiy-storchaka commented Jul 30, 2024

Uh oh!

Uh oh!

pygeek commented Jul 8, 2024 •

edited by bedevere-app bot

Loading

pygeek commented Jul 8, 2024 •

edited

Loading

pygeek commented Jul 10, 2024 •

edited

Loading