BUG/RFC: CarbonserverListener.fetchData always returns a nil error leading to unreachable code

[drawks]

While looking at some issues with the concurrency in this method I noticed that while an effort is made to keep track of and even log on errors

go-carbon/carbonserver/render.go

Line 627 in e5c35e1

} else {

the final return always returns nil

go-carbon/carbonserver/render.go

Line 638 in e5c35e1

return multi, nil

which means that calling code which checks for and handles errors is unreachable

go-carbon/carbonserver/render.go

Lines 445 to 450 in e5c35e1

	if err != nil {
	listener.accessLogger.Error("error while fetching the data",
	zap.Error(err),
	)
	continue
	}

I looked back through the commit history and I don't see where the caller ever used the error for controlling business logic, so I presume that the current behavior is expected, even if it isn't necessarily intended or desired.

A simple fix of returning the first error or returning a "multierror" with errors.Join(errs...) would cause a behavioral change in that the continue would now be reachable and would prevent any results from being passed into the result channel.

go-carbon/carbonserver/render.go

Line 449 in e5c35e1

continue

Overall I suspect that this entire code path could/should be rewritten to avoid some of ambiguity. Perhaps it would make more sense to pass the result channel directly to the go routines in a sort of multiproducer pattern and we could also remove some of mutex banging which brought me to this code to begin with 🤷🏽

At any rate, I'm looking for some comments/feedback before I float an MR which introduces any behavioral change