coredns / rrl

Response Rate Limiting Plugin for CoreDNS
Apache License 2.0
23 stars 21 forks source link

shard.Add() fails to call s.Unlock() if eviction fails #36

Closed markdingo closed 2 years ago

markdingo commented 2 years ago

I was reviewing cache/cache.go and I notice that if s.evict() fails, then shard.Add() returns without calling s.Unlock() here:

https://github.com/coredns/rrl/blob/1ebc54edd477cb399328e57470c0920f47205cc6/plugins/rrl/cache/cache.go#L161-L162

Can I suggest that the idiomatic s.Lock(); defer s.Unlock() might be a safer way of dealing with this?

As an aside, this has probably never triggered a lock stall (that is a go routine attempting to relock the same shard) because there also appears to be a bug in the eviction function registered by RRL.initTable() - which suggests to me that eviction may always succeed when sometimes it should not. I'll post a separate issue on that.