Skip to content

Fix list modification during iteration in wait_for_threads_to_finish#1471

Closed
juandiego-bmu wants to merge 1 commit intoOWASP:masterfrom
juandiego-bmu:fix-unsafe-list-iteration
Closed

Fix list modification during iteration in wait_for_threads_to_finish#1471
juandiego-bmu wants to merge 1 commit intoOWASP:masterfrom
juandiego-bmu:fix-unsafe-list-iteration

Conversation

@juandiego-bmu
Copy link
Copy Markdown

Summary

Replace threads.remove(thread) inside for thread in threads loop with a list comprehension slice assignment.

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.

Split from #1466 as requested by @pUrGe12.

Fixes #1465

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
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a28423dc-66a8-43f4-9985-b99c1929aa33

📥 Commits

Reviewing files that changed from the base of the PR and between f4c1bbf and da75232.

📒 Files selected for processing (1)
  • nettacker/core/utils/common.py

Summary by CodeRabbit

  • Bug Fixes
    • Improved thread management stability by correcting the thread filtering logic to prevent potential issues during concurrent thread operations.

Walkthrough

The wait_for_threads_to_finish() function in nettacker/core/utils/common.py was modified to filter alive threads using an in-place list comprehension instead of removing elements during iteration, which previously caused the iterator to skip elements and delayed thread cleanup.

Changes

Cohort / File(s) Summary
Thread Synchronization
nettacker/core/utils/common.py
Replaced iterative removal pattern in wait_for_threads_to_finish() with in-place list filtering using threads[:] = [t for t in threads if t.is_alive()], eliminating iterator skipping during list mutation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: fixing unsafe list modification during iteration in the wait_for_threads_to_finish function.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem (removing elements during iteration shifts the iterator) and the solution (using list comprehension with slice assignment).
Linked Issues check ✅ Passed The PR implements the exact fix suggested in issue #1465 by replacing threads.remove(thread) with threads[:] = [t for t in threads if t.is_alive()], addressing both the iterator-shifting problem and ensuring accurate thread count checks.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the list modification bug during iteration in wait_for_threads_to_finish; no out-of-scope modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes unsafe mutation of the threads list during iteration in wait_for_threads_to_finish, preventing skipped elements when multiple threads/processes finish between checks.

Changes:

  • Replace in-loop threads.remove(thread) while iterating with an in-place slice assignment that filters to alive items (threads[:] = [...]).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@securestep9
Copy link
Copy Markdown
Collaborator

@juandiego-bmu please follow the contributor guidelines and PR template checks. It might be easier if you create a new clean PR which follows the guidelines and checks reference it here and close this PR.

@juandiego-bmu
Copy link
Copy Markdown
Author

Closing to recreate with proper PR template and signed commits as requested by @securestep9.

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.

[Bug] List modification during iteration in wait_for_threads_to_finish skips threads

3 participants