fix(runner): makes PreProcessTarget concurrent#1556
Conversation
by launching goroutines. `*Runner.PreProcessTargets` prev executed work inline, so sizedwaitgroup's `Add()` blocking effectively serialized target processing. Prefixing the worker with `go` restores true concurrency capped by `Threads` opt. Signed-off-by: Dwi Siswanto <git@dw1.io>
WalkthroughPreProcessTargets now processes each target concurrently by launching a goroutine per target and synchronizing with an existing WaitGroup. The internal AddTarget invocation remains, but execution is parallelized across targets. No exported function signatures were modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Runner
participant WG as WaitGroup
participant T1 as Goroutine(Target 1)
participant Tn as Goroutine(Target N)
participant Add as AddTarget
Caller->>Runner: PreProcessTargets(targets)
Runner->>WG: Add(len(targets))
loop for each target
Runner->>T1: spawn go func(target)
T1->>Add: AddTarget(target)
T1-->>WG: Done()
Runner->>Tn: spawn go func(target)
Tn->>Add: AddTarget(target)
Tn-->>WG: Done()
end
Runner->>WG: Wait()
WG-->>Runner: all done
Runner-->>Caller: return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/runner/targets.go (1)
106-115: Enforce Threads >= 1 — sizedwaitgroup.New(size <= 0) uses math.MaxInt32 (practically unbounded)pkg/runner/targets.go (lines 106–115) launches a goroutine per scanned target with wg.Add(); sizedwaitgroup.New(size <= 0) sets the limit to math.MaxInt32 (≈2^31), enabling effectively-unbounded concurrency. Clamp/validate Threads to a positive sane default before creating the sizedwaitgroup (e.g., if Threads <= 0 { Threads = 1 }).
🧹 Nitpick comments (2)
pkg/runner/targets.go (2)
117-119: Handle scanner errors after the loopbufio.Scanner errors (e.g., token too long, I/O) are currently ignored. Check s.Err() after Wait() and return it.
Apply this diff:
wg.Wait() + if err := s.Err(); err != nil { + return err + } return nil
96-96: Optional: guard Threads to sane boundsIf Threads can be 0/negative (interpreted as unlimited) or extremely large, consider clamping to [1, some upper cap] to avoid DNS flood and goroutine storms.
Example:
+ import "runtime" @@ - wg := sizedwaitgroup.New(r.options.Threads) + threads := r.options.Threads + if threads <= 0 { + threads = runtime.GOMAXPROCS(0) + } + if threads > 4096 { + threads = 4096 + } + wg := sizedwaitgroup.New(threads)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/runner/targets.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build-windows
- GitHub Check: build-mac
- GitHub Check: build-linux
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Lint Test
- GitHub Check: release-test-linux
- GitHub Check: release-test-windows
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
pkg/runner/targets.go (2)
121-218: Verify thread-safety of IPRanger mutationspkg/runner/targets.go (AddTarget) now calls IPRanger.Contains, AddHostWithMetadata and Np.ValidateAddress concurrently. IPRanger implementation was not found in the repo, so cannot confirm these methods are goroutine-safe — verify that Contains/AddHostWithMetadata and Np.ValidateAddress are concurrency-safe; if not, add internal locking (sync.Mutex / sync.RWMutex) or funnel all mutations through a single worker/channel.
93-96: Close is safe — PreProcessTargets is the sole producerAll sends to r.streamChannel are inside pkg/runner/targets.go (lines 134, 143, 156, 189, 191, 197). PreProcessTargets waits for worker goroutines (wg.Wait at line 117) before returning, so defer close(r.streamChannel) runs after all sends complete; no other producers found in repo.
by launching goroutines.
*Runner.PreProcessTargetsprev executed workinline, so sizedwaitgroup's
Add()blockingeffectively serialized target processing.
Prefixing the worker with
gorestores trueconcurrency capped by
Threadsopt.Note
./naabu-75db5d2 (dev) -- Fix host discovery (#1553)
./naabu-pr-1555 -- PR #1555.
./naabu -- this patch.
Fixes #1540
Summary by CodeRabbit