*: ban the go keyword in non-test code

[jordanlewis]

Misuse of the go keyword is problematic for a server that desires to be HA like CockroachDB: a panic-causing stack on the resultant goroutine will crash the entire process without custom crash handling.

There are already several wrappers for go throughout the codebase, such as Tasks and Workers creatable by the stopper. These wrappers perform cleanup and report errors to sentry, and can be extended over time. Unfortunately, they're not used consistently, and their use is not enforced.

This ticket tracks removing the go keyword from non-test code, except for the packages that implement the wrappers, and adding a linter that bans direct use of the go keyword except for in those places.

Jira issue: CRDB-3425

[jordanlewis]

Here is the current list of users of go in the source tree. It's possible not all of these would be appropriately replaced by stopper.RunAsyncTask or whatever the case may be. I would advocate to add a simpler utils.Go() method that performs cleanup, dumps goroutines and currently-active queries to a file, and reports to Sentry, but perhaps some of these ought to be migrated to a stopper. I'm not sure.

rg -tgo -g "!{*_test,fuzz,*.pb.gw}.go" -g"!pkg/{workload,cmd,cli,acceptance,testutils}" -g"!pkg/util/stop"  "\Wgo func" pkg | pbcopy

pkg/sql/virtual_table.go:	go func() {
pkg/sql/planhook.go:	go func() {
pkg/sql/colexec/parallel_unordered_synchronizer.go:		go func(input SynchronizerInput, inputIdx int) {
pkg/sql/copy_file_upload.go:	go func() {
pkg/sql/pgwire/conn.go:	go func() {
pkg/sql/pgwire/server.go:	go func() {
pkg/sql/create_stats.go:	go func() {
pkg/sql/stats/stats_cache.go:	go func() {
pkg/sql/execinfra/base.go:			go func(input RowSource) {
pkg/sql/flowinfra/inbound.go:	go func() {
pkg/sql/flowinfra/flow_scheduler.go:	go func() {
pkg/sql/flowinfra/flow_registry.go:				go func(r InboundStreamHandler) {
pkg/sql/flowinfra/flow_registry.go:	go func() {
pkg/sql/flowinfra/flow.go:		go func(i int) {
pkg/sql/flowinfra/flow.go:	go func() {
pkg/sql/flowinfra/flow.go:		go func(receiver InboundStreamHandler) {
pkg/sql/rowflow/routers.go:		go func(ctx context.Context, rb *routerBase, ro *routerOutput, wg *sync.WaitGroup) {
pkg/sql/internal.go:	go func() {
pkg/sql/colflow/colrpc/outbox.go:	go func() {
pkg/sql/colflow/vectorized_flow.go:			go func() {
pkg/rpc/context.go:	go func() {
pkg/rpc/context.go:	go func() {
pkg/jobs/deprecated.go:		go func() {
pkg/jobs/deprecated.go:		go func() {
pkg/jobs/adopt.go:		go func(id int64) {
pkg/jobs/adopt.go:	go func() {
pkg/jobs/adopt.go:	go func() {
pkg/server/status.go:		go func() {
pkg/server/drain.go:	go func() {
pkg/ccl/sqlproxyccl/server.go:	go func() {
pkg/ccl/sqlproxyccl/server.go:		go func() {
pkg/ccl/changefeedccl/changefeed_processors.go:	go func() {
pkg/ccl/sqlproxyccl/proxy.go:			go func() {
pkg/ccl/sqlproxyccl/proxy.go:	go func() {
pkg/ccl/sqlproxyccl/proxy.go:	go func() {
pkg/ccl/importccl/import_processor.go:	go func() {
pkg/ccl/backupccl/backup_processor.go:	go func() {
pkg/ccl/backupccl/split_and_scatter_processor.go:	go func() {
pkg/util/netutil/net.go:		go func() {
pkg/util/log/clog.go:		go func() {
pkg/util/hlc/hlc.go:	go func() {
pkg/security/certificate_manager.go:	go func() {
pkg/util/tracing/grpc_interceptor.go:	go func() {
pkg/util/sdnotify/sdnotify_unix.go:	go func() {
pkg/util/sdnotify/sdnotify_unix.go:	go func() {

[knz]

I have looked at a few. I believe we need a mix of both. A large number can+should use a stopper and a context.Context, which will also ensure they are properly shut down. A few of them would need something smaller, so I'd be in favor of adding utils.Go() as well.

[knz]

@tbg this connects well to our discussion earlier last week to promote the use of the stopper to ensure that logging contexts get properly propagated.

[tbg]

Thanks! I wasn't aware of this issue, glad it already exists.First I wanted to add that we'd also have to do something about errgroup, ctxgroup, etc. These generally expose a .Go method, so it shouldn't be so hard to lint against them as well (and they're also straightforward enough to wrap).

My (unordered) running list of benefits for reworking our Stopper to be the go replacement primitive is the following (makes no attempts to avoid duplicating arguments mentioned above):

1. can recover/report panics automatically (caveat: more work needed to recover so without causing further issues like tagging mutex critical sections etc)
2. integration with background tracing
3. possible path towards Span memory reuse (since we can prevent spans from crossing goroutine boundaries)
4. can automatically assign goroutine labels
5. ^-- enables a better implementation of trace-based deterministic goroutine testing as spearheaded by @nvanbenschoten here and possibly allows us to write many more tests in this fashion
6. removing the log singleton (one log instance per stopper, handed to each task via ctx)
7. avoid test failures where the stopper stops but an RPC handler still fires and runs into, e.g. panic("pebble: closed").

[tbg]

I am looking at our existing use of profiler labels (https://rakyll.org/profiler-labels/) right now and what it would take to make them usable for observability - say you're looking at a node that hovers at 95% CPU, what operation causes the CPU to be consumed? I think that in the short term, profiler labels are our best bet at developing a solution here.

Profiler labels are interesting because they transition into child goroutines, meaning that if we do a good job at labeling goroutines "where the work starts" and making sure that these labels hop across RPCs, we should be in a fairly good place. Right now, it's good enough so that if some SQL operation is burning CPU, you will be able to pick out which one it is. But what isn't yet possible (as far as I can tell) is that if you start with an overloaded CPU, you can tell from the profiles conclusively that a statement is responsible for it. Sure, you see some CPU attributed to that statement, but not, like, the lion's share. It seems that most of the work done on behalf of the statement is elsewhere. I still have to understand why that is, but it's possible that it ties in with this issue. One issue is definitely that when you delegate work to a thread pool (as happens for raft operations), the association breaks. But at least for read-heavy workloads, I'm not aware of such a switching point. Possibly DistSQL has one, though.

[ajwerner]

#98269 is another example of code where a panic shouldn't bring down the node. Distsql flows should recover panics.