[BEAM-10206] Remove Fatalf calls in non-test goroutines for tests/benchmarks

[jrmccluskey]

Replace goroutines calling Fatalf in tests/benchmarks to address go vet warnings.

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Choose reviewer(s) and mention them in a comment (R: @username).
• Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

ValidatesRunner compliance status (on master branch)

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---					---
Java
Python	---					---
XLang						---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

[codecov]

Codecov Report

Merging #16575 (056aec8) into master (ef07638) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #16575   +/-   ##
=======================================
  Coverage   46.32%   46.32%           
=======================================
  Files         201      201           
  Lines       19734    19734           
=======================================
  Hits         9141     9141           
  Misses       9612     9612           
  Partials      981      981           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef07638...056aec8. Read the comment docs.

[jrmccluskey]

R: @lostluck
CC: @riteshghorse @youngoli

First run at fixing these. Had a lot of trouble with blocking on channel reads piping out of the goroutine, but found a suggestion to replace it with Errorf() + immediate return out instead.

[lostluck]

I gather the issue vet/staticcheck has is that the "fatal" doesn't end the test?

Since usually seeing the combo of "error " return means one must translate it back to fatal, consider adding another comment to the effect about the goroutines, and the vet warning. I assume though that this fix lets staticcheck and vet run cleanly on this issue.

Alternatively to unambiguously avoid the different lint trigger: Add a new buffered channel errchan that takes in a string. Then send the message and close the channel (though a defer close(errchan) at the top of the goroutine is also good.

Receive on the errchan at the end of the benchmark function (after closing process), and print out any out put to b.Fatal. (Yes we lose line locality, but we can make that up in message verbosity). This then blocks the benchmark until the goroutine shuts down, which will close the channel and unblock exit.

close(process)
for msg := range errchan {
  b.Fatal(msg)
}

The rule of thumb here is "the channel writer should close the channel".

[jrmccluskey]

Well looks like the issue with the blocking was because I was checking before close(process) was called. I like the channel method better because it's a little more clear how the error is handled compared to the error + return.

[lostluck]

You should still have the "return" after these.

[jrmccluskey]

Fixed