Skip to content

Commit

Permalink
stmtdiagnostics: fix possible race when polling is disabled
Browse files Browse the repository at this point in the history
This commit fixes a possible race on `timeutil.Timer.C` that can happen
when polling interval is disabled. In particular, the race would occur
because disabling the polling `Stop`s the timer which puts it back into
the pool which disallows further access to the `Timer` object. However,
we already stored the reference to `Timer.C`, so we'd read from the
channel which could be concurrently overwritten by another goroutine
(that happened to pick up this timer object from the pool). To solve
this race we now store the channel separately and explicitly nil it out
after having stopped the timer (which makes it block forever, until the
polling interval is changed).

Additionally, this commit adds non-negative validation to the cluster
setting since there is really no point in allowing negative durations
for the polling interval.

Release note: None
  • Loading branch information
yuzefovich committed Jul 11, 2023
1 parent 27e36a0 commit 049c29a
Showing 1 changed file with 15 additions and 5 deletions.
20 changes: 15 additions & 5 deletions pkg/sql/stmtdiagnostics/statement_diagnostics.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ var pollingInterval = settings.RegisterDurationSetting(
settings.TenantReadOnly,
"sql.stmt_diagnostics.poll_interval",
"rate at which the stmtdiagnostics.Registry polls for requests, set to zero to disable",
10*time.Second)
10*time.Second,
settings.NonNegativeDuration,
)

var bundleChunkSize = settings.RegisterByteSizeSetting(
settings.TenantWritable,
Expand Down Expand Up @@ -158,19 +160,27 @@ func (r *Registry) Start(ctx context.Context, stopper *stop.Stopper) {

func (r *Registry) poll(ctx context.Context) {
var (
timer timeutil.Timer
timer timeutil.Timer
// We need to store timer.C reference separately because timer.Stop()
// (called when polling is disabled) puts timer into the pool and
// prohibits further usage of stored timer.C.
timerC = timer.C
lastPoll time.Time
deadline time.Time
pollIntervalChanged = make(chan struct{}, 1)
maybeResetTimer = func() {
if interval := pollingInterval.Get(&r.st.SV); interval <= 0 {
// Setting the interval to a non-positive value stops the polling.
if interval := pollingInterval.Get(&r.st.SV); interval == 0 {
// Setting the interval to zero stops the polling.
timer.Stop()
// nil out the channel so that it'd block forever in the loop
// below (until the polling interval is changed).
timerC = nil
} else {
newDeadline := lastPoll.Add(interval)
if deadline.IsZero() || !deadline.Equal(newDeadline) {
deadline = newDeadline
timer.Reset(timeutil.Until(deadline))
timerC = timer.C
}
}
}
Expand All @@ -195,7 +205,7 @@ func (r *Registry) poll(ctx context.Context) {
select {
case <-pollIntervalChanged:
continue // go back around and maybe reset the timer
case <-timer.C:
case <-timerC:
timer.Read = true
case <-ctx.Done():
return
Expand Down

0 comments on commit 049c29a

Please sign in to comment.