sql: possible memory leak in Streamer

[nvanbenschoten]

In each call to NewStreamer, we register an OnChange handler on the streamerConcurrencyLimit:

cockroach/pkg/kv/kvclient/kvstreamer/streamer.go

Lines 338 to 340 in 2bc16c3

	streamerConcurrencyLimit.SetOnChange(&st.SV, func(ctx context.Context) {
	s.coordinator.asyncSem.UpdateCapacity(uint64(streamerConcurrencyLimit.Get(&st.SV)))
	})

This handler captures the Streamer object s. The handler is then never unregistered. I believe this leaks the Streamer object and all references. This is a problem because, unlike most static objects that are leaked due to calls to SetOnChange, we create a new Streamer for each SQL query that uses one.

Two ways to address this are to:

1. unregister the OnChange handler when tearing down the Streamer. We don't currently have a way to unregister an OnChange handler, but adding one should not be difficult.
2. skip the OnChange handler. Do we need a single instance of a Streamer to react dynamically to cluster setting changes? Is it enough for new instances to use the new limit?

Note: I haven't actually confirmed that this leaks memory, so determining whether it's real would be the first step here.

[yuzefovich]

Thanks Nathan for spotting this! I believe you're correct that we have a memory leak (although it's not huge because Streamer objects are nil-ed out in Streamer.Close, so we're only leaking the object itself and nothing else). I agree with your point that we don't need to dynamically react to a change in the cluster setting, so I'm removing that in #76480.