Fix list modification during iteration in wait_for_threads_to_finish#1498
Fix list modification during iteration in wait_for_threads_to_finish#1498juandiego-bmu wants to merge 3 commits intoOWASP:masterfrom
Conversation
Replace threads.remove(thread) inside for loop with a list comprehension slice assignment. Removing elements during iteration shifts the iterator and can skip threads that just finished. Fixes OWASP#1465
Summary by CodeRabbit
WalkthroughReplaced in-loop removal of non-alive threads with an atomic filtered reassignment in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency bug in wait_for_threads_to_finish() where removing elements from the threads list during iteration could skip completed threads, causing delayed cleanup and inaccurate maximum checks.
Changes:
- Replace in-loop
threads.remove(thread)with a safe in-place list rebuild (threads[:] = [...]) to avoid iterator skipping.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def wait_for_threads_to_finish(threads, maximum=None, terminable=False, sub_process=False): | ||
| while threads: | ||
| try: | ||
| for thread in threads: | ||
| if not thread.is_alive(): | ||
| threads.remove(thread) | ||
| threads[:] = [t for t in threads if t.is_alive()] | ||
| if maximum and len(threads) < maximum: |
There was a problem hiding this comment.
The bugfix changes thread cleanup semantics; there’s currently no regression test coverage for wait_for_threads_to_finish, even though tests/core/utils/test_common.py exists for other common.py utilities. Please add a unit test that exercises the skipped-element scenario (e.g., multiple completed threads adjacent in the list) and validates maximum behavior (breaking once len(threads) < maximum).
|
@juandiego-bmu can you please add a unit test (see Copilot comments) |
Address review feedback: add docstring and unit tests covering dead-thread cleanup, the maximum parameter, and in-place list mutation -- confirming the fix prevents skipping elements.
|
Done! Pushed a commit with:
All tests pass locally. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/utils/test_common.py (1)
118-132:maximumtest should assert early-exit semantics explicitly.Right now it only checks
True, so it can still pass if the function waits for full completion.🔧 Suggested test tightening
def test_wait_for_threads_to_finish_with_maximum(): """Should break early when thread count drops below maximum.""" - - def short_task(): - time.sleep(0.02) - - threads = [threading.Thread(target=short_task) for _ in range(3)] + release = threading.Event() + + def short_task(): + time.sleep(0.01) + + def blocking_task(): + release.wait(timeout=1) + + threads = [ + threading.Thread(target=short_task), + threading.Thread(target=short_task), + threading.Thread(target=blocking_task), + threading.Thread(target=blocking_task), + ] for t in threads: t.start() result = common_utils.wait_for_threads_to_finish(threads, maximum=3) assert result is True + assert len(threads) == 2 + assert all(t.is_alive() for t in threads) # Clean up + release.set() for t in threads: t.join(timeout=1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/utils/test_common.py` around lines 118 - 132, The test test_wait_for_threads_to_finish_with_maximum currently only asserts True, which can pass even if the helper waited for all threads; modify it to assert early-exit by making short_task take longer (e.g., time.sleep(0.2)), call common_utils.wait_for_threads_to_finish(threads, maximum=3, timeout=0.1) (or use a small timeout/poll) and then assert result is True AND at least one thread in threads is still alive (e.g., any(t.is_alive() for t in threads)), then perform cleanup joins; reference symbols: test_wait_for_threads_to_finish_with_maximum, short_task, threads, common_utils.wait_for_threads_to_finish.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/utils/test_common.py`:
- Around line 118-132: The test test_wait_for_threads_to_finish_with_maximum
currently only asserts True, which can pass even if the helper waited for all
threads; modify it to assert early-exit by making short_task take longer (e.g.,
time.sleep(0.2)), call common_utils.wait_for_threads_to_finish(threads,
maximum=3, timeout=0.1) (or use a small timeout/poll) and then assert result is
True AND at least one thread in threads is still alive (e.g., any(t.is_alive()
for t in threads)), then perform cleanup joins; reference symbols:
test_wait_for_threads_to_finish_with_maximum, short_task, threads,
common_utils.wait_for_threads_to_finish.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ec2cdca-bd6e-4312-b598-42d431965955
📒 Files selected for processing (2)
nettacker/core/utils/common.pytests/core/utils/test_common.py
Proposed change
Replace
threads.remove(thread)insidefor thread in threadsloop with a list comprehension slice assignment inwait_for_threads_to_finish()(nettacker/core/utils/common.py).Removing elements during iteration shifts the iterator and can skip threads that just finished. Using
threads[:] = [t for t in threads if t.is_alive()]rebuilds the list safely.Linked issue: #1465
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folderRecreated from #1471 with signed commits and proper template as requested by @securestep9.