diff --git a/plugins/processors/ifname/ifname.go b/plugins/processors/ifname/ifname.go index ad633a0ae2900..e9fb8df4d208a 100644 --- a/plugins/processors/ifname/ifname.go +++ b/plugins/processors/ifname/ifname.go @@ -111,8 +111,7 @@ type IfName struct { gsBase snmp.GosnmpWrapper `toml:"-"` - sigs sigMap `toml:"-"` - sigsLock sync.Mutex `toml:"-"` + sigs sigMap `toml:"-"` } const minRetry time.Duration = 5 * time.Minute @@ -251,14 +250,14 @@ func (d *IfName) getMap(agent string) (entry nameMap, age time.Duration, err err } // Is this the first request for this agent? - d.sigsLock.Lock() + d.rwLock.Lock() sig, found := d.sigs[agent] if !found { s := make(chan struct{}) d.sigs[agent] = s sig = s } - d.sigsLock.Unlock() + d.rwLock.Unlock() if found { // This is not the first request. Wait for first to finish. @@ -281,24 +280,21 @@ func (d *IfName) getMap(agent string) (entry nameMap, age time.Duration, err err m, err = d.getMapRemote(agent) if err != nil { //failure. signal without saving to cache - d.sigsLock.Lock() + d.rwLock.Lock() close(sig) delete(d.sigs, agent) - d.sigsLock.Unlock() + d.rwLock.Unlock() return nil, 0, fmt.Errorf("getting remote table: %w", err) } - // Cache it + // Cache it, then signal any other waiting requests for this agent + // and clean up d.rwLock.Lock() d.cache.Put(agent, m) - d.rwLock.Unlock() - - // Signal any other waiting requests for this agent and clean up - d.sigsLock.Lock() close(sig) delete(d.sigs, agent) - d.sigsLock.Unlock() + d.rwLock.Unlock() return m, 0, nil } diff --git a/plugins/processors/ifname/ifname_test.go b/plugins/processors/ifname/ifname_test.go index 8eff68006f99b..85ddc767411c0 100644 --- a/plugins/processors/ifname/ifname_test.go +++ b/plugins/processors/ifname/ifname_test.go @@ -26,7 +26,7 @@ func TestTable(t *testing.T) { config := snmp.ClientConfig{ Version: 2, - Timeout: internal.Duration{Duration: 5 * time.Second}, // doesn't work with 0 timeout + Timeout: internal.Duration{Duration: 5 * time.Second}, // Doesn't work with 0 timeout } gs, err := snmp.NewWrapper(config) require.NoError(t, err) @@ -36,7 +36,7 @@ func TestTable(t *testing.T) { err = gs.Connect() require.NoError(t, err) - //could use ifIndex but oid index is always the same + // Could use ifIndex but oid index is always the same m, err := buildMap(gs, tab, "ifDescr") require.NoError(t, err) require.NotEmpty(t, m) @@ -53,7 +53,7 @@ func TestIfName(t *testing.T) { CacheSize: 1000, ClientConfig: snmp.ClientConfig{ Version: 2, - Timeout: internal.Duration{Duration: 5 * time.Second}, // doesn't work with 0 timeout + Timeout: internal.Duration{Duration: 5 * time.Second}, // Doesn't work with 0 timeout }, } err := d.Init() @@ -93,65 +93,52 @@ func TestIfName(t *testing.T) { func TestGetMap(t *testing.T) { d := IfName{ - SourceTag: "ifIndex", - DestTag: "ifName", - AgentTag: "agent", CacheSize: 1000, - ClientConfig: snmp.ClientConfig{ - Version: 2, - Timeout: internal.Duration{Duration: 5 * time.Second}, // doesn't work with 0 timeout - }, - CacheTTL: config.Duration(10 * time.Second), + CacheTTL: config.Duration(10 * time.Second), } - // This test mocks the snmp transaction so don't run net-snmp - // commands to look up table names. + // Don't run net-snmp commands to look up table names. d.makeTable = func(agent string) (*si.Table, error) { return &si.Table{}, nil } err := d.Init() require.NoError(t, err) - // Request the same agent multiple times in goroutines. The first - // request should make the mocked remote call and the others - // should block until the response is cached, then return the - // cached response. - expected := nameMap{ 1: "ifname1", 2: "ifname2", } - var wgRemote sync.WaitGroup var remoteCalls int32 - wgRemote.Add(1) + // Mock the snmp transaction d.getMapRemote = func(agent string) (nameMap, error) { atomic.AddInt32(&remoteCalls, 1) - wgRemote.Wait() //don't return until all requests are made return expected, nil } + m, age, err := d.getMap("agent") + require.NoError(t, err) + require.Zero(t, age) // Age is zero when map comes from getMapRemote + require.Equal(t, expected, m) - const thMax = 3 - var wgReq sync.WaitGroup + // Remote call should happen the first time getMap runs + require.Equal(t, int32(1), remoteCalls) + var wg sync.WaitGroup + const thMax = 3 for th := 0; th < thMax; th++ { - wgReq.Add(1) + wg.Add(1) go func() { - defer wgReq.Done() - m, _, err := d.getMap("agent") + defer wg.Done() + m, age, err := d.getMap("agent") require.NoError(t, err) + require.NotZero(t, age) // Age is nonzero when map comes from cache require.Equal(t, expected, m) }() } - //signal mocked remote call to finish - wgRemote.Done() - - //wait for requests to finish - wgReq.Wait() + wg.Wait() - //remote call should only happen once + // Remote call should not happen subsequent times getMap runs require.Equal(t, int32(1), remoteCalls) - }