refactor: replace Split in loops with more efficient SplitSeq - #7278
Conversation
Signed-off-by: stringsbuilder <stringsbuilder@outlook.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces eager string splitting with sequence-based iteration across parsing, protocol, template, and integration-test paths. ChangesString Parsing Refactoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/protocols/network/network.go (1)
202-213:⚠️ Potential issue | 🟡 MinorTrim whitespace before parsing each port token.
Line 206 will reject a common input like
port: "80, 443"because the second token still includes leading whitespace.✂️ Suggested fix
- for port := range strings.SplitSeq(request.Port, ",") { + for rawPort := range strings.SplitSeq(request.Port, ",") { + port := strings.TrimSpace(rawPort) if port == "" { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/network/network.go` around lines 202 - 213, When iterating tokens from strings.SplitSeq(request.Port, ",") trim whitespace from each token before using it: use a local variable (e.g., tok := strings.TrimSpace(port)), skip if tok == "", parse tok with strconv.Atoi, validate its range, and append the trimmed token (tok) to request.ports (not the original port with whitespace). Update the error messages to include tok where appropriate so parsing failures reflect the trimmed value; keep references to request.Port, strings.SplitSeq, strconv.Atoi, and request.ports.
🧹 Nitpick comments (2)
pkg/tmplexec/flow/flow_executor.go (1)
302-311: This still materializes the entire helper file before tokenization.Line 302's
io.ReadAll(reader)dominates the allocation profile here, and the substrings appended in Lines 306-310 keep that full backing string alive. If this path is part of the perf goal, scan the reader directly instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tmplexec/flow/flow_executor.go` around lines 302 - 311, The code currently calls io.ReadAll(reader) and then iterates over strings.SplitSeq(string(bin), "\n"), which materializes the entire reader into memory and keeps the large backing string alive; replace that pattern by scanning the reader directly with a bufio.Scanner (or bufio.Reader.ReadString/ReadBytes loop) to read lines one-by-one from reader, trim each scanned line (strings.TrimSpace), and append non-empty lines to values, and if necessary configure Scanner.Buffer to handle long lines; update the logic that references reader, values, and strings.SplitSeq to use the scanner instead.pkg/installer/template.go (1)
568-575: There's still a per-record split allocation in this loop.Line 570 only needs two fields, but the full split still creates a slice for every checksum entry. A two-part split would align better with the allocation-reduction goal of this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/installer/template.go` around lines 568 - 575, The per-record allocation comes from using strings.Split on each checksum line (variable v) and assigning to tmparr; replace that with a two-field split (e.g., strings.SplitN(v, ",", 2) or better strings.Cut(v, ",")) to avoid allocating a full slice for every entry, then validate the second field exists before assigning into allChecksums[tmparr[0]] = tmparr[1] (or using the two return values from strings.Cut) while keeping the surrounding loop and trimming logic the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/protocols/network/network.go`:
- Around line 202-213: When iterating tokens from strings.SplitSeq(request.Port,
",") trim whitespace from each token before using it: use a local variable
(e.g., tok := strings.TrimSpace(port)), skip if tok == "", parse tok with
strconv.Atoi, validate its range, and append the trimmed token (tok) to
request.ports (not the original port with whitespace). Update the error messages
to include tok where appropriate so parsing failures reflect the trimmed value;
keep references to request.Port, strings.SplitSeq, strconv.Atoi, and
request.ports.
---
Nitpick comments:
In `@pkg/installer/template.go`:
- Around line 568-575: The per-record allocation comes from using strings.Split
on each checksum line (variable v) and assigning to tmparr; replace that with a
two-field split (e.g., strings.SplitN(v, ",", 2) or better strings.Cut(v, ","))
to avoid allocating a full slice for every entry, then validate the second field
exists before assigning into allChecksums[tmparr[0]] = tmparr[1] (or using the
two return values from strings.Cut) while keeping the surrounding loop and
trimming logic the same.
In `@pkg/tmplexec/flow/flow_executor.go`:
- Around line 302-311: The code currently calls io.ReadAll(reader) and then
iterates over strings.SplitSeq(string(bin), "\n"), which materializes the entire
reader into memory and keeps the large backing string alive; replace that
pattern by scanning the reader directly with a bufio.Scanner (or
bufio.Reader.ReadString/ReadBytes loop) to read lines one-by-one from reader,
trim each scanned line (strings.TrimSpace), and append non-empty lines to
values, and if necessary configure Scanner.Buffer to handle long lines; update
the logic that references reader, values, and strings.SplitSeq to use the
scanner instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb4462ab-2d3f-4a47-917f-e710b053336f
📒 Files selected for processing (10)
cmd/integration-test/http.gointernal/runner/options_test.gopkg/catalog/config/nucleiconfig.gopkg/catalog/config/template.gopkg/installer/template.gopkg/js/devtools/tsgen/parser.gopkg/protocols/javascript/js.gopkg/protocols/network/network.gopkg/testutils/integration.gopkg/tmplexec/flow/flow_executor.go
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
themavik
left a comment
themavik
left a comment
There was a problem hiding this comment.
SplitSeq/FieldsSeq churn is straightforward. nit: a few call sites still use strings.Split for small inner splits (e.g. installer checksum line) — not wrong, just inconsistent if you are chasing allocs everywhere.
|
@Mzack9999 @dogancanbakir Hi, Could you please review this PR at your convenience? Thank you very much. |
|
Holding review until #7419 is merged. The |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/runner/preflight_portscan.go (1)
264-269: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake per-port counter initialization atomic.
Concurrent first hits can both observe
nil, create separate counters, and overwrite one another inperPortOpen, causing the open-port distribution summary to undercount. Preinitialize counters forportsToScanor guard the Get/Create/Set sequence with a mutex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runner/preflight_portscan.go` around lines 264 - 269, Make the perPortOpen counter setup in the port-scan aggregation path atomic so concurrent first hits cannot overwrite each other. Prefer preinitializing one atomic counter for every port in portsToScan before processing results, or protect the existing Get/Create/Set sequence with a mutex; preserve the subsequent counter.Add(1) behavior.pkg/installer/template.go (1)
586-593: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winParse checksum entries at the final comma.
writeChecksumFileInDirstores raw paths with no escaping, sostrings.Split(v, ",")will discard any entry whose path contains a comma. Use the last comma as the separator instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/installer/template.go` around lines 586 - 593, Update the checksum parsing loop in writeChecksumFileInDir to split each entry at the final comma rather than using strings.Split, preserving commas within the path while separating it from the checksum. Continue skipping entries that lack a valid separator and store the parsed path and checksum in allChecksums.
🧹 Nitpick comments (1)
internal/runner/preflight_portscan.go (1)
200-206: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove or consume the unused
allIPsaggregation.The code builds and sorts every resolved IP, but
allIPsis never used by the subsequent per-target scan. For large inputs this adds unnecessary O(n) memory and O(n log n) sorting overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runner/preflight_portscan.go` around lines 200 - 206, Remove the unused allIPsMap/allIPs aggregation and its sort.Strings call from the preflight scan setup. Keep allIPsSet available for the subsequent per-target scan, and remove any imports that become unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/runner/preflight_portscan.go`:
- Around line 264-269: Make the perPortOpen counter setup in the port-scan
aggregation path atomic so concurrent first hits cannot overwrite each other.
Prefer preinitializing one atomic counter for every port in portsToScan before
processing results, or protect the existing Get/Create/Set sequence with a
mutex; preserve the subsequent counter.Add(1) behavior.
In `@pkg/installer/template.go`:
- Around line 586-593: Update the checksum parsing loop in
writeChecksumFileInDir to split each entry at the final comma rather than using
strings.Split, preserving commas within the path while separating it from the
checksum. Continue skipping entries that lack a valid separator and store the
parsed path and checksum in allChecksums.
---
Nitpick comments:
In `@internal/runner/preflight_portscan.go`:
- Around line 200-206: Remove the unused allIPsMap/allIPs aggregation and its
sort.Strings call from the preflight scan setup. Keep allIPsSet available for
the subsequent per-target scan, and remove any imports that become unused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36295919-c145-4487-b004-473a643d2b2c
📒 Files selected for processing (9)
internal/runner/preflight_portscan.gointernal/tests/integration/http_test.gopkg/catalog/config/nucleiconfig.gopkg/catalog/config/template.gopkg/installer/template.gopkg/protocols/javascript/js.gopkg/protocols/network/network.gopkg/templates/fuzz_harness.gopkg/tmplexec/flow/flow_executor.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/protocols/javascript/js.go
- pkg/catalog/config/template.go
- pkg/catalog/config/nucleiconfig.go
- internal/tests/integration/http_test.go
Avoid shared templates-dir update races across parallel go test packages and ignore known leveldb/ratelimit/memguardian goroutines in goleak.
Keep SplitSeq inputs in locals so iterator substrings cannot outlive a temporary string expression.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/tests/sdk_test.go (1)
23-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the package-wide goleak suppressions.
IgnoreAnyContainingPkghides every goroutine stack containing the expirable LRU, goleveldb, or ratelimit package paths—not only the documented long-lived workers. Prefer exact goroutine-function ignores, or verify that this broad suppression is intentional and covered separately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/tests/sdk_test.go` around lines 23 - 30, Narrow the goleak suppressions in the SDK leak test by replacing the package-wide IgnoreAnyContainingPkg entries for expirable LRU, goleveldb, and ratelimit with exact goroutine-function ignores for only their documented long-lived workers. Keep the existing memguardian function-specific suppression, and verify each replacement targets the intended worker rather than unrelated goroutines from the same package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/tests/sdk_test.go`:
- Around line 23-30: Narrow the goleak suppressions in the SDK leak test by
replacing the package-wide IgnoreAnyContainingPkg entries for expirable LRU,
goleveldb, and ratelimit with exact goroutine-function ignores for only their
documented long-lived workers. Keep the existing memguardian function-specific
suppression, and verify each replacement targets the intended worker rather than
unrelated goroutines from the same package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bbcac1f-5fd3-42bf-bae2-88e6cca2db2b
📒 Files selected for processing (3)
lib/tests/sdk_test.gopkg/installer/template.gopkg/templates/fuzz_harness.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/installer/template.go
- pkg/templates/fuzz_harness.go
Keep goleak ignores, drop DisableUpdateCheck so CI can install templates, and serialize UpdateIfOutdated across processes.
3 participants
Proposed changes
Closes #7561
strings.SplitSeq (introduced in Go 1.24) returns a lazy sequence, allowing iteration over tokens one by one without creating an intermediate slice.
It significantly reduces memory allocations and can improve performance for long strings.
More info: golang/go#61901
Proof
Checklist
Summary by CodeRabbit
NoErrorassertions, plus refined goroutine-leak ignores.