Skip to content

Commit 861c882

Browse files
authored
[-] fix QueryMeasurements nil pointer dereference, fixes #766 (#767)
* keep existing connection in SourceConns.SyncFromReader() Since `SourceConn` passed to reapMetricMeasurements() it's crucial to keep the existing source with connection alive, so the reaper keeps it's work. And there is no need to close the connection because it might be used right now in some metric fetcher. Let Reaper to cancel all disabled monitored sources in the main loop of Reap() function. * fix tests
1 parent 874e7e5 commit 861c882

File tree

4 files changed

+4
-10
lines changed

4 files changed

+4
-10
lines changed

internal/reaper/cache.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func GetMonitoredDatabaseByUniqueName(ctx context.Context, name string) (*source
3232
monitoredDbCacheLock.RLock()
3333
defer monitoredDbCacheLock.RUnlock()
3434
md, exists := monitoredDbCache[name]
35-
if !exists || md == nil {
35+
if !exists || md == nil || md.Conn == nil {
3636
return nil, fmt.Errorf("database %s not found in cache", name)
3737
}
3838
return md, nil

internal/reaper/reaper.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,6 @@ func (r *Reaper) ShutdownOldWorkers(ctx context.Context, hostsToShutDownDueToRol
264264
r.CloseResourcesForRemovedMonitoredDBs(hostsToShutDownDueToRoleChange)
265265
}
266266

267-
// metrics.ControlMessage notifies of shutdown + interval change
268267
func (r *Reaper) reapMetricMeasurements(ctx context.Context, md *sources.SourceConn, metricName string) {
269268
hostState := make(map[string]map[string]string)
270269
var lastUptimeS int64 = -1 // used for "server restarted" event detection

internal/sources/conn.go

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -276,20 +276,16 @@ func (mds SourceConns) SyncFromReader(r Reader) (newmds SourceConns, err error)
276276
return nil, err
277277
}
278278
newmds, err = srcs.ResolveDatabases()
279-
for _, newMD := range newmds {
279+
for i, newMD := range newmds {
280280
md := mds.GetMonitoredDatabase(newMD.Name)
281281
if md == nil {
282282
continue
283283
}
284284
if reflect.DeepEqual(md.Source, newMD.Source) {
285-
// keep the existing connection if the source is the same
286-
newMD.Conn = md.Conn
287-
newMD.ConnConfig = md.ConnConfig
285+
// replace with the existing connection if the source is the same
286+
newmds[i] = md
288287
continue
289288
}
290-
if md.Conn != nil {
291-
md.Conn.Close()
292-
}
293289
}
294290
return newmds, err
295291
}

internal/sources/conn_test.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,6 @@ func TestMonitoredDatabases_SyncFromReader(t *testing.T) {
240240
assert.NotNil(t, mdbs, "ResolveDatabases() = nil, want not nil")
241241
// pretend that we have a connection
242242
mdbs[0].Conn = db
243-
db.ExpectClose()
244243
// sync the databases and make sure they are the same
245244
newmdbs, _ := mdbs.SyncFromReader(reader)
246245
assert.NotNil(t, newmdbs)

0 commit comments

Comments
 (0)