There's a "fatal error: concurrent map iteration and map write" race condition tripping up the build. Using profiler labels to figure out where we're waiting around rather than registering tasks with the stopper makes sense. Was that the original motivation behind the heavy-weight task tracking?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @irfansharif, @knz, and @tbg)

pkg/util/stop/stopper.go, line 240 at r2 (raw file):

}

func (s *Stopper) refuseRLocked() bool {

nit: give this guy a comment. What is it refusing?

pkg/util/stop/stopper.go, line 309 at r2 (raw file):

// Returns an error to indicate that the system is currently quiescing and
// function f was not called.
func (s *Stopper) RunTask(ctx context.Context, taskName string, f func(context.Context)) error {

Do we want to keep taking the taskName? I guess it doesn't hurt.

pkg/util/stop/stopper.go, line 433 at r2 (raw file):

	s.mu.RLock()
	defer s.mu.RUnlock()
	atomic.AddInt32(&s.mu.numTasks, -1)

Do we need to protect numTasks with the RLock? Seems like we could just use atomics to read it in NumTasks with an atomic.LoadInt32.

pkg/util/stop/stopper.go, line 440 at r2 (raw file):

	s.mu.RLock()
	defer s.mu.RUnlock()
	return int(s.mu.numTasks)

This looks wrong. We either need the exclusive lock to read without an atomic load, or we need an atomic load, in which case, why the lock at all?

Related to the previous comment.

pkg/util/stop/stopper.go, line 536 at r2 (raw file):

Is the idea to avoid the log under the exclusive lock?

pkg/util/stop/stopper.go, line 544 at r2 (raw file):

	s.mu.qCancels = nil

	for s.mu.numTasks > 0 {

Can we read this without a lock and without an atomic load?

Was that the original motivation behind the heavy-weight task tracking?

No, profiler labels didn't even exist then. I think the original intention was that the stopper would be orchestrating a node-wide drain process cleanly and that we'd need deep visibility into it. But that hasn't at all become true, the stopper is really just supposed to tear things down as quickly as possible without leaking goroutines, that's pretty much it, and if it fails to do so it's a bug and will need staring at goroutine dumps anyway.

That said, I have been looking into improving our use of profiler labels in CPU and goroutine profiles with the goal of making CPU profiles a useful tool for assigning blame for CPU usage. Concretely, SREs see a CPU-pegged cluster/node, let them find the app, user, or stmt that does it.

Thanks for the reviews, and sorry about the shoddy PR. Should be better now :)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @irfansharif, and @knz)

pkg/util/stop/stopper.go, line 309 at r2 (raw file):

I've intentionally stayed away from taskName so far, though I think it should ultimately go. What I think will replace it is profiler labels. For example, all goroutines associated to a client session should have the goroutine label for that (with propagation across RPC boundaries). Something like the task name is easy to remove but hard to add back, so I'll wait until that actually materializes.

pkg/util/stop/stopper.go, line 433 at r2 (raw file):

Added a comment on the field:

	// _numTasks is the number of active tasks. It is accessed atomically
	// via addTask() under the read lock for task acquisition. We need the
	// read lock to ensure task creation is prohibited atomically with the
	// quiescing or stopping bools set below.

pkg/util/stop/stopper.go, line 536 at r2 (raw file):

That was the idea, but I don't like the looks of this either. I'm now using an AfterFunc to only log the line after 5s, which I think is the right compromise between not logging anything in the normal case, and making sure we can tell that we're stuck during draining if we are.

pkg/util/stop/stopper.go, line 544 at r2 (raw file):

No, I had only half a brain to work with when I posted this PR. Should've queued it for the AM.

I am personally attached to task names but not the task map. I believe task names give insight into the current process load (once we can inspect the tasks, which will come). I also believe in pprof labels but I think these serve a different need.

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen and @irfansharif)

Reviewed 1 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @irfansharif, @knz, and @tbg)

pkg/util/stop/stopper.go, line 309 at r2 (raw file):

👍 that sounds like the right approach.

pkg/util/stop/stopper.go, line 433 at r2 (raw file):

But do we need the read lock in runPostlude() or in NumTasks()? As you mention in that comment, the read lock ensures that task creation is prohibited atomically with the quiescing or stopping bools set below.

pkg/util/stop/stopper.go, line 437 at r4 (raw file):

	s.mu.RLock()
	defer s.mu.RUnlock()
	return int(s.addTaskRLocked(0))

This seems like an anti-pattern. An atomic.AddInt32(0) is very different than an atomic.LoadInt32() at the microarchitecture level.

Thanks for the reviews!

bors r=knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @irfansharif, and @knz)

pkg/util/stop/stopper.go, line 433 at r2 (raw file):

Oh, that's a good call. No, not needed. Fixed.

pkg/util/stop/stopper.go, line 437 at r4 (raw file):

NumTasks doesn't need to be fast, so I don't know about the anti-pattern (isn't premature optimization an anti-pattern?), but I'm with you in spirit, done.