Should we maybe have a more detailed description here?
Something like "NodeScanHandler keeps track of all arriving NodeScanV2 messages and sends them to central for processing"

Out of curiosity: Is there a reason you moved the var up here?

I think I had a variant of a solution that was using this channel. But this is a leftover from it and it can be moved back to where it was.

I think I understand that the intent of this code is to arrange a graceful shutdown.
c.stopC is when the shutdown was requested. c.stoppedC is when the handler actually shut down.

I understand that you likely copied the pattern found in command_handler_impl.go, but I think the pattern is wrong.

This specific fragment will just make for loop repeat quickly eating up CPU until c.nodeScans channel is closed.

I believe the implementation should be fixed here and propagated to other handlers.

I find https://github.com/stackrox/stackrox/blob/master/sensor/common/sensor/central_sender_impl.go to be more clean. Hopefully it can serve as a reference implementation.

A unit test for this handler will be very handy to demonstrate and validate its behavior.

I find https://github.com/stackrox/stackrox/blob/master/sensor/common/sensor/central_sender_impl.go to be more clean. Hopefully it can serve as a reference implementation.

I would love to follow up on this, as I see there several issues that makes the concurrent code difficult to follow. These are not no-goes, but some patterns that I learnt are violated. Examples:

• forwardResponses writes to a channel to that is passed as an external parameter and never closes the channel to signal that there will no more messages.
• componentMsgsC is never closed, so why to have the code that handles the situation when the channel is closed?
• forwardResponses stops working as soon as for !s.stopC.IsDone() { - this is the same situation as here, we drop messages on the floor instead of forwarding them, as we do not have any guarantee that the stream will still be up.

I am not saying that the code is not working, but I have several rules that I follow when working with concurrency:

• the goroutine that writes to a channel is responsible for creating the channel and closing it,
• only the writer knows that there will be no more messages sent to a channel. If that happens, the channel should be closed to inform all readers that there will be no more messages,
• there is exactly one writer goroutine for a channel, but there may be many readers.

Having said that, I am curious to learn why do you think that the code sensor/common/sensor/central_sender_impl.go is cleaner.

This specific fragment will just make for loop repeat quickly eating up CPU until c.nodeScans channel is closed.

Yes, this is an issue and should be addressed. I guess adding return should do the job, but I will check that.

A unit test for this handler will be very handy to demonstrate and validate its behavior.

Agreed! Will try to produce some tests for this.

Your notes about channels are likely right. I did not check how channels are handled but I definitely saw that some of them aren't closed.

I brought central_sender_impl.go because it more accurately deals with c.stopC and c.stoppedC than this code. Particularly, there should be only one place that sets c.stoppedC - the deferred hook. Everything else like error conditions or closed channels should just initiate the shut down by setting c.stopC. Signalling that the shutdown completed (which c.stoppedC is supposed to do) in the middle of the function seems wrong to me.

This has been addressed in 2077d2a

Maybe just NodeScans?

In regards to V2: You're absolutely right about V1 not being a real NodeScan (src), but as the existing feature is called NodeScan in every piece of currently used code, it might be very confusing to introduce a completely different thing under the same name, right?

I will make a proposal to clear up the naming and their meaning, as I see that it makes even more problems when navigating though the code of Central and Scanner.

I don't mind keeping V2. In this case I'd vote to have V2 more consistently. There are some spots in this PR where it could be (re-)added.

Follow-up to the naming discussion: #3755

Nitpicking, but there's nothing besides the test that calls Stopped() function. Why have it at all?

It is gone now.

It seems to me, that Start() can only be called once. At the moment when run() completes either due to Stop() or due to error, c.stopC will be set. Any subsequent attempts to Start() will not fail (maybe they should?) and will lead to run() quickly exiting.

In such situation, I think it makes sense to defer close c.toCentral so that we properly close channel and eliminate one forwarding goroutine (see central_sender_impl.go).
Please take into account that select/case will choose a random branch if multiple are satisfied, therefore it makes sense to additionally guard for { loop with a check. E.g. for !c.stopC.IsDone() {.

Got it covered!

I am only not fully sure which goroutine is meant with "eliminate one forwarding goroutine" 🤔

This seems brittle. What if after a bad refactoring (in the future) you give it more time and it receives all 1000 messages? What if 50 milliseconds are not enough to receive even 1 message on some systems?
I have no concrete ideas for improvement but it feels the test tries to do too much.

Correct. I fixed that in a bit less flaky (but not ideal) way.

How come no goroutine leaks if the one started by nodeScansProducerThatStops.start() should hang and remain blocked after sending the first few items over its channel?

Correct, it leaked. This should be fixed now (checked with https://github.com/uber-go/goleak, which I cannot include as the test logger used here leaks one goroutine)

I played more with the leaks and the goleak from Uber. I was able to eliminate all leaks in the code (test & feat). I hope it is okay to add https://github.com/uber-go/goleak as a new dependency.

I think this is a go1.19 thing. Since we are updating the file anyway, might as well keep it, but I have also had the displeasure of dealing with this when trying to style this repo

Yeah, I saw that and reverted to go 1.18

Up to you, but want to keep this as a Debug log if that'll help development?

We will work more on the sensor corner, so there will be a chance to add some more meaningful debugging

curious, do we ever send to the other channels? why is this the only receive-only channel?

I am following a pattern where I explicitly identify writers and readers by using general channel in the first case and the read-only in the second case. Here, this helps us to discover early potential errors when the reader would try to write to a channel.

uh oh. is this a bad leak? I see it's unresolved, but I wonder how much this affects us

I didn't dig deeper, but there is some info on the link added. I think the leak is in a dependency of a dependency of a dependency and it may be tricky to get rid of.

since we have a suite, maybe we should use s.Assert().NoError(h.Start())? Doesn't really matter tbh, though

Oh, sorry, you are right, this would look nicer. I spotted your review first after merging, but I will apply the suggestions in the next PR in the stack (#3755) or in a separate PR.

How is this run in CI?

Good question. I need to check whether this is included in the race tests.

Does this imply there is a plan to add this (I'm imagining Central requesting a scan periodically as part of reprocessing or user forcing it)?

This is added to implement the handler interface. It does not meant that we will support anything arriving from central, but we do have such possibility of we want to :)

Do we want to allow multiple Start calls to happen?

For example, this seems possible:

Start()
Start()
Start()
...
Stop()

I investigated and this will cause a panic due to closing a channel twice (only when the feature-flag is enabled). I will provide a fix and a test for this.

up to you, but maybe we can do something like the following to remove all the indentation:

if scan == nil {
        return
}
...

do we need the return here and for the other case?

No, no need for them. I will remove them.

what is this used for?

This seems to be a leftover.