fix(test): avoid restarting finished processes in batch subtest#84
Open
okurz wants to merge 3 commits into
Open
fix(test): avoid restarting finished processes in batch subtest#84okurz wants to merge 3 commits into
okurz wants to merge 3 commits into
Conversation
Motivation: t/02_parallel.t's 'batch' subtest called `$c->start()` on the whole Pool after adding a 3rd process. Pool proxies `start()` to every member, so the 2 already-finished processes got re-forked too, racing with the new process's output under CI resource constraints (observed failing on macOS latest-Perl runners). Design Choices: use `$c->last->start()` to start only the newly added process, matching the pattern already used later in the same subtest. Benefits: removes unnecessary re-forking and the associated race, making the test deterministic. Related issue: #83 (review)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
=======================================
Coverage 91.60% 91.61%
=======================================
Files 45 45
Lines 2848 2849 +1
=======================================
+ Hits 2609 2610 +1
Misses 239 239 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
perlpunk
approved these changes
Jul 6, 2026
Motivation: t/13_shared.t is flaky under concurrency (e.g. macOS CI runners): Semaphore::_create() looks up an existing semaphore, and if missing, creates one with IPC_CREAT|IPC_EXCL. When multiple forked processes race to create the same semaphore, the loser's create call fails with EEXIST and confess()es, killing that child instead of just using the semaphore the winner created. Reproduced locally: 30 processes racing to create the same semaphore key fail ~15-35% of the time; after the fix, 0/30 across repeated runs. Design Choices: on a failed create, fall back to a lookup instead of failing outright, since a failed create in this code path can only mean another process won the race and already created it. Benefits: removes a source of spurious child-process deaths under concurrent semaphore/lock creation, without changing the semaphore's external behavior. Related issue: https://github.com/openSUSE/Mojo-IOLoop-ReadWriteProcess/actions/runs/28792986622/job/85376167573?pr=84
b8fecea to
463ec35
Compare
Member
Author
|
refactored code to ensure full coverage |
Martchus
requested changes
Jul 7, 2026
| # Try acquiring already existing semaphore | ||
| my $sem = IPC::Semaphore->new($key, $self->count, 0); | ||
| unless (defined $sem) { | ||
| # Try creating it. IPC_EXCL in flags fails with EEXIST if it already |
Collaborator
There was a problem hiding this comment.
This sentence isn't really comprehensible. What is "it" referring to? Maybe:
Suggested change
| # Try creating it. IPC_EXCL in flags fails with EEXIST if it already | |
| # Try acquiring a possibly existing semaphore using the IPC_EXCL flag. This will fail with EEXIST if it already |
Motivation: t/13_shared.t was flaky on macOS CI runners, failing with incorrect pid counts and stale process attachments. IPC::SharedMem objects do not automatically detach from shared memory when garbage- collected, causing persistent attachment leaks in the parent and child processes that eventually corrupt and exhaust shared resources. Design Choices: added an explicit detach() call to Memory::DESTROY wrapped in an eval block, ensuring any active shared memory attachments are safely released whenever a shared_memory instance goes out of scope, while still executing cleanups (remove) when destroy is set. Benefits: completely eliminates persistent shared memory attachment leaks, ensuring clean test isolation, resource reclamation, and stable execution under concurrent/stress environments.
Martchus
approved these changes
Jul 9, 2026
Collaborator
|
Although it would still be nice to improve the wording as mentioned before. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix 1: t/02_parallel.t 'batch' subtest race (motivation for this PR)
CI for #83 failed on
t/02_parallel.tline 74 (macOS, latest Perl):Root cause: after adding a 3rd process,
$c->start()was called on the wholePool, which proxies to every member, re-forking the 2 already-finishedprocesses and racing with the new process's output. Fixed by using
$c->last->start()to start only the newly added process (matching thepattern already used later in the same subtest).
Fix 2: t/13_shared.t semaphore-creation race
After fix 1, CI still failed in
t/13_shared.twith pid counts off from the expected 20/21.Root cause:
Shared::Semaphore::_create()looks up an existing semaphore and,if missing, creates one with
IPC_CREAT|IPC_EXCL. When several forkedprocesses race to create the same semaphore concurrently, the losing
process's create call fails with
EEXISTandconfess()s, killing thatchild instead of just attaching to the semaphore the winner created.
Fix: on a failed create, fall back to a lookup instead of failing (optimistically create first, fall back on failure to attach).
Fix 3: t/13_shared.t shared memory attachment resource leak
After fix 2, some runs under high resource contention / stress still failed in
t/13_shared.twithshm_nattch > 0assertions.Root cause:
IPC::SharedMemobjects do not automatically detach from their shared memory segments upon garbage collection when they go out of scope. They only detach ifdetach()is explicitly called. This resulted in a cumulative resource leak of active memory attachments in the parent process from previous subtests (such asfree_memor manual locks), causingshm_nattch == 0assertions to fail.Fix: Added an explicit
detach()call wrapped inevalinsideMemory::DESTROY, ensuring all active attachments are cleaned up automatically as soon as ashared_memoryhelper instance goes out of scope.Testing
perl -Ilib t/02_parallel.tpasses repeatedly.TEST_SHARED=1 perl -Ilib t/13_shared.tpasses repeatedly (under extreme concurrent load + background stress testing).TEST_SHARED=1 TEST_SUBREAPER=1 prove -l t(full suite) passes repeatedly.perltidyreports no formatting changes needed for modified files.