Fix data race in xfr.go#8039
Conversation
| z.RLock() | ||
| apex, err := z.apexIfDefinedLocked() | ||
| t := z.Tree | ||
| z.RUnlock() |
There was a problem hiding this comment.
I am evaluating whether z.ApexIfDefined() should expand to include z.Tree so that we don't have to do this awkward manual lock.
That would (naively) be a backwards-incompatible change, happy to take advice from maintainers
There was a problem hiding this comment.
Alternately I could keep z.ApexIfDefined() unchanged and have it internally delegate to a new private method that returns apex, tree, err
There was a problem hiding this comment.
IMO the current approach is fine. It's consistent with how Lookup already snapshots under a single read lock:
z.RLock()
ap := z.Apex
tr := z.Tree
z.RUnlock()Difference is that Transfer needs the assembled []dns.RR slice. I think the locking intention is clear here, and it's only for a single call site.
thevilledev
left a comment
There was a problem hiding this comment.
Good catch! Looks like it's been there for quite a while.
Can you fix the DCO? Other than that I think it's ready to go
| z.RLock() | ||
| apex, err := z.apexIfDefinedLocked() | ||
| t := z.Tree | ||
| z.RUnlock() |
There was a problem hiding this comment.
IMO the current approach is fine. It's consistent with how Lookup already snapshots under a single read lock:
z.RLock()
ap := z.Apex
tr := z.Tree
z.RUnlock()Difference is that Transfer needs the assembled []dns.RR slice. I think the locking intention is clear here, and it's only for a single call site.
Signed-off-by: Ryan Brewster <rpb@anthropic.com>
16ffee2 to
f717dae
Compare
|
Great, hopefully fixed DCO |
1. Why is this pull request needed and what does it do?
Fixes a data race in xfr.go
2. Which issues (if any) are related?
#8038
pretty reliably triggers a race for me on master, and with this fix it reliably passes
3. Which documentation changes (if any) need to be made?
none
4. Does this introduce a backward incompatible change or deprecation?
none