Fetchers to return their own channel

[amirbenun]

Motivation

As we know, sending on a close channel causes a panic. Therefore the best practices in golang is that the sender always the one to close the channel. In our posture flavor, that's not the case, all the fetchers are sharing the same channel which none of them can close. Instead, each fetcher should have full ownership on its channel. One way of doing that is to modify the fetcher interface to return a receive-only channel.

type Fetcher interface {
    Fetch(context.Context, CycleMetadata) (chan <-fetching.ResourceInfo, error)
    Stop()
}

Then the manager that trigger all the fetchers will mux the results into a single channel and return it chan <-fetching.ResourceInfo.

Definition of done

• [ ] All fetchers should return a new receive-only channel
• [ ] Once a fetcher has completed it can close the channel
• [ ] A manager should mux all fetchers into a single channel
• [ ] When all fetchers has completed the manager can close the channel

Related tasks/epics

Reference related issues and epics

[orestisfl]

One way of doing that is to modify the fetcher interface to return a receive-only channel.

But this means creating one channel on each fetching cycle?

[orestisfl]

https://stackoverflow.com/questions/8593645/is-it-ok-to-leave-a-channel-open

In my understanding, there is no need to close channels. It's neither expected or required for garbage collection. close() is an extra signal meaning that the sender is done sending information. In our architecture, the senders do not decide when they are done sending so closing the channel is a not a meaningful action.

I vote to base lifespan decisions solely on the context object and I repeat my view on how to implement this safely: https://github.com/elastic/cloudbeat/issues/691#issuecomment-1638330693

[amirbenun]

In our system, the resources channel remains open and isn't disposed of by the garbage collector since it's still in use. Instead, the fetchers can indicate the completion of their cycle by closing the channel. This action would subsequently close channels up to the "posture" layer.

While the idea of creating the ContextAwareChannel is appealing, there are significant distinctions between the two suggestions. Introducing this implementation won't necessarily ensure developers adopt this practice consistently. My concern is its sporadic usage within our codebase. It's crucial to address this aspect for the suggestion to be complete.

Nonetheless, the proposed changes align with Golang best practices. More importantly, they are architectural adjustments that will obligate future contributors to these components to maintain these standards.

[orestisfl]

In our system, the resources channel remains open and isn't disposed of by the garbage collector since it's still in use. Instead, the fetchers can indicate the completion of their cycle by closing the channel. This action would subsequently close channels up to the "posture" layer.

That's why I propose to use contexts to signal when the reader(s) should exit, in that case the resource channel will have zero references and will be garbage collected.

While the idea of creating the ContextAwareChannel is appealing, there are significant distinctions between the two suggestions. Introducing this implementation won't necessarily ensure developers adopt this practice consistently. My concern is its sporadic usage within our codebase. It's crucial to address this aspect for the suggestion to be complete.

It will be enforced if we modify our interfaces accordingly. Specifically, by not exposing raw channels in fetchers.