[WIP & POC] Refactor concurrency into simpler errorgroup design by tonymet · Pull Request #141 · Ullaakut/nmap

tonymet · 2025-07-16T20:10:44Z

hi @Ullaakut big thanks this project has been a huge help for a vulners scanner I built.

The general goal is to reduce state, channels and concurrency to make debugging the core parsing engine more intuitive.

High level changes

favor io.Reader over []byte and reduce bulk copying
read stdout &stderr with just 2 goroutines instead of many goroutines and channels
use errgroup over waitgroup for simpler design.
clean up unnecessary pointers to reduce crashes

Benefits

clean out most goroutines and channels
reduced state and logic to improve debugging.

Broken

progress bar support is pending
rawXML is missing. plan to replace with io.Reader instead of []byte
[new] test suite is mostly failing due to mismatch return types e.g. []string{nil} / []string{}

I understand it's a major reworking of the core component. If it's desirable, I can restore the missing functionality into a proper PR. If it's too big a change, I'm happy to close it out.

Ullaakut · 2025-07-17T08:22:28Z

Amazing work @tonymet ! Luck would have it, I just started rewriting https://github.com/Ullaakut/cameradar and was going to make some fundamental changes to this library as well along the way, so I am extremely glad you beat me to it 😄

IMO progress bar support can be added back in a later PR, that is not a huge deal for me especially considering how adding that feature contributed to reducing code quality IIRC.

Ullaakut

Overall lots of good things, but this highlights how much of my code is massively outdated, non-idiomatic and overall garbage quality and needs a deeper rewrite.

If you want we could join our efforts and I can join you on this PR by committing to your branch and making further changes as well for what I had planned. Let me know if that works for you, would love your feedback on my changes 👍

Ullaakut · 2025-07-17T08:25:38Z

+	if err := cmd.Start(); err != nil {
+		return result, warnings, err
+	} else if warnings, err := s.processNmapResult(s.ctx, &result, stdoutPipe, stderrPipe); err != nil {
+		return result, warnings, err
+	} else if err := cmd.Wait(); err != nil {
+		return result, warnings, err


No need for elses here, since we return in the other statements. Also personally, I am fine with inlining error checks only when the error is the only thing returned, bit of a personal preference there :p

Also I would rather add a bit more context to the errors while we're at it.

Suggested change

if err := cmd.Start(); err != nil {

return result, warnings, err

} else if warnings, err := s.processNmapResult(s.ctx, &result, stdoutPipe, stderrPipe); err != nil {

return result, warnings, err

} else if err := cmd.Wait(); err != nil {

return result, warnings, err

if err := cmd.Start(); err != nil {

return result, warnings, fmt.Errorf("starting command: %w", err)

}

warnings, err := s.processNmapResult(s.ctx, &result, stdoutPipe, stderrPipe)

if err != nil {

return result, warnings, fmt.Errorf("processing output: %w", err)

}

if err := cmd.Wait(); err != nil {

return result, warnings, fmt.Errorf("waiting for command: %w", err)

WDYT?

the if / else if / else is more of a personal style thing to keep variables scoped to the if {} blocks . it can look clumsy in some of these. I defer to your style here

Ullaakut · 2025-07-17T08:26:31Z

 	} else {
-		err = s.processNmapResult(result, warnings, &stdout, &stderr, done, doneProgress)
+		return result, warnings, err
 	}


Suggested change

} else {

err = s.processNmapResult(result, warnings, &stdout, &stderr, done, doneProgress)

return result, warnings, err

}

return result, warnings, nil

If I'm reading the diff correctly, getting here means err is nil so this is clearer IMO

Ullaakut · 2025-07-17T08:27:16Z

+		if warnings, err = checkStdErr(stderr); err != nil {
+			return err
+		} else {


Same comment here, no need for the else since we return in the error case already.

Suggested change

if warnings, err = checkStdErr(stderr); err != nil {

return err

} else {

if warnings, err = checkStdErr(stderr); err != nil {

return fmt.Errorf("stderr: %w", err)

} else {

good suggestion!

Ullaakut · 2025-07-17T08:28:34Z

-	for _, warning := range stderrSplit {
-		warning = strings.Trim(warning, " ")
-		*warnings = append(*warnings, warning)
+	var warnings = make([]string, 0)


Suggested change

var warnings = make([]string, 0)

var warnings []string

If we don't initialize it with a predetermined size, IMO it's better to just use the zero value this way

Ullaakut · 2025-07-17T08:30:04Z

+	for scanner.Scan() {
+		warning := scanner.Text()
+		warnings = append(warnings, warning)
 		switch {
 		case strings.Contains(warning, "Malloc Failed!"):
-			return ErrMallocFailed
+			return warnings, ErrMallocFailed
 		case strings.Contains(warning, "requires root privileges."):
-			return ErrRequiresRoot
-		// TODO: Add cases for other known errors we might want to guard.
-		default:
+			return warnings, ErrRequiresRoot


Suggested change

for scanner.Scan() {

warning := scanner.Text()

warnings = append(warnings, warning)

switch {

case strings.Contains(warning, "Malloc Failed!"):

return ErrMallocFailed

return warnings, ErrMallocFailed

case strings.Contains(warning, "requires root privileges."):

return ErrRequiresRoot

// TODO: Add cases for other known errors we might want to guard.

default:

return warnings, ErrRequiresRoot

for scanner.Scan() {

warning := scanner.Text()

switch {

case strings.Contains(warning, "Malloc Failed!"):

return warnings, ErrMallocFailed

case strings.Contains(warning, "requires root privileges."):

return warnings, ErrRequiresRoot

}

warnings = append(warnings, warning)

Doing it in this order avoids having the warnings also contain the error we're returning for, seems more logical to me wdyt?

Ullaakut · 2025-07-17T08:32:20Z


 type testStreamer struct{}

+var ctx, cancel = context.WithTimeout(context.Background(), 5*time.Second)


Should not be a global if not absolutely necessary (in this case, it isn't)

Since go 1.24 we have access to t.Context which should be used in any new code imo, and no need to add an extra timeout anymore, this will cleanup the components nicely at the end of the test

Ullaakut · 2025-07-17T08:34:35Z

+	dat, err := os.Open("tests/xml/scan_base.xml")
 	if err != nil {
 		panic(err)
 	}


Suggested change

dat, err := os.Open("tests/xml/scan_base.xml")

if err != nil {

panic(err)

}

dat, err := os.Open("tests/xml/scan_base.xml")

require.NoError(t, err)

If you dont mind, I can make a pass on your branch to uniformize/modernize/fix all of the tests as well, might be quicker than by going through PR comments since there is a lot to change 😁

Ullaakut · 2025-07-17T08:37:14Z

+	if contentAll, err := io.ReadAll(content); err != nil {
+		return err
+	} else {
+		return xml.Unmarshal(contentAll, result)
+	}


Suggested change

if contentAll, err := io.ReadAll(content); err != nil {

return err

} else {

return xml.Unmarshal(contentAll, result)

}

b, err := io.ReadAll(content)

if err != nil {

return err

}

return xml.Unmarshal(b, result)

tonymet · 2025-07-17T16:05:18Z

great feedback and I love the enthusiasm. It might be best to have a chat about your goals and I can see how to best contribute changes.

If you are currently planning a major overhaul, I would recommend keeping this PR as a draft and pulling these concepts into your larger work until your changes stabilize. Many of the changes in this PR are not quite ready to merge and were done just to stabilize the branch for testing with my app. the core functionality of Run , processNmapResult , Parse seem to work well

but this highlights how much of my code is massively outdated, non-idiomatic and overall garbage quality and needs a deeper rewrite.

I recently had a similar experience with gcloud-go -- the original version had tons of goroutines, channels, workers etc to manage concurrent uploads -- and then I realized I was overthinking concurrency patterns in golang and replaced it with 80% fewer lines using errgroup.

tonymet · 2025-08-21T21:25:10Z

just checking in on how you'd like to proceed. if you'd like to integrate manually or you could apply your changes first and i could rebase mine on top

Ullaakut · 2025-08-28T05:42:28Z

Sorry, lots of stuff going on IRL, I have no time for open source work for now unfortunately. I'll let you decide on how you want to handle this for now 👍 Should be back in a few weeks.

tonymet · 2025-08-28T16:01:27Z

no worries and thanks for the updates. let's check when you get back. take care!

Ullaakut · 2026-01-27T16:34:21Z

Superseded by #145 (sorry for the delay @tonymet !)

tonymet added 5 commits July 16, 2025 08:55

major refactor to reduce complexity, memory and goroutines

302b3ef

unify test context timout

0da4ff1

cleanup result ptr and warnings to reduce null ptr

d48fccf

github actions update deps

07af195

cleanup warnings

a49dfca

Ullaakut requested changes Jul 17, 2025

View reviewed changes

Ullaakut assigned tonymet Jul 17, 2025

Ullaakut added the enhancement New feature or request label Jul 17, 2025

fix empty warning message

6efa333

Ullaakut closed this Jan 27, 2026


		type testStreamer struct{}

		var ctx, cancel = context.WithTimeout(context.Background(), 5*time.Second)

Conversation

tonymet commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High level changes

Benefits

Broken

Uh oh!

Ullaakut commented Jul 17, 2025

Uh oh!

Ullaakut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonymet commented Jul 17, 2025

Uh oh!

tonymet commented Aug 21, 2025

Uh oh!

Ullaakut commented Aug 28, 2025

Uh oh!

tonymet commented Aug 28, 2025

Uh oh!

Ullaakut commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tonymet commented Jul 16, 2025 •

edited

Loading