copy rrs #4416
Merged
copy rrs #4416
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
miekg
commented
Jan 21, 2021
miekg
commented
- Revert "make copies of RRs before returning them (plugin/file: Make copies of RRs before returning them to avoid tree mutation #4409)"
- Document copying responses
This reverts commit 8b2ff6c.
See #4409 and the comments. This documents that issue, but doesn't change the in-tree plugins just yet. Signed-off-by: Miek Gieben <[email protected]>
miekg
requested review from
bradbeam,
chrisohaver,
dilyevsky,
Isolus,
johnbelamaric,
pmoroney,
rajansandeep,
stp-ip,
SuperQ and
yongtang
as code owners
|
cc @chrisohaver |
|
Is there a potential write/read race condition here when a plugin such as file or auto is updating an RR in the tree, at the same time the response writer is reading that RR? |
|
[ Quoting <[email protected]> in "Re: [coredns/coredns] copy rrs (#44..." ]
Is there a potential write/read race condition here when a plugin such as file
or auto is updating an RR in the tree, at the same time the response writer is
reading that RR?
likely yes... :-( so maybe not revert?
/Miek
…--
Miek Gieben
|
|
[ Quoting <[email protected]> in "Re: [coredns/coredns] copy rrs (#44..." ]
Is there a potential write/read race condition here when a plugin such as file
or auto is updating an RR in the tree, at the same time the response writer is
reading that RR?
although file doesn't update a single RR (no dynamic updates)
|
Right - I suppose it replaces the entire tree, leaving the previous tree to be GC'ed. I see in file that only the tree/apex pointers are locked, not the contents of the trees. So, I think it's safe as is. Ultimately, putting the responsibility of fixing this on response writers seems better because it only copies when needed. |
chrisohaver
reviewed
Jan 21, 2021
|
[ Quoting <[email protected]> in "Re: [coredns/coredns] copy rrs (#44..." ]
although file doesn't update a single RR (no dynamic updates)
Right - I suppose it replaces the entire tree, leaving the previous tree to be
GC'ed. I see in file that only the tree/apex pointers are locked, not the
contents of the trees. So, I think it's safe as is.
Ultimately, putting the responsibility of fixing this on response writers seems
better because it only copies when needed.
Agreed
|
Co-authored-by: Chris O'Haver <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #4416 +/- ##
==========================================
- Coverage 55.35% 55.34% -0.01%
==========================================
Files 223 223
Lines 9938 9936 -2
==========================================
- Hits 5501 5499 -2
Misses 3979 3979
Partials 458 458
Continue to review full report at Codecov.
|
|
still needs fixed in rewrite |
was expecting that to be added to this pr before it merged ... |
|
@miekg , Should we open an issue so we don't forget about it? Or do you have a fix in the works already? |
|
[ Quoting <[email protected]> in "Re: [coredns/coredns] copy rrs (#44..." ]
still needs fixed in rewrite
was expecting that to be added to this pr before it merged ...
I suspect more plugins we need something, so didn't want to conflate and enlarge this
(which was essentally a revert only)
|
|
[ Quoting <[email protected]> in "Re: [coredns/coredns] copy rrs (#44..." ]
@miekg , Should we open an issue so we don't forget about it? Or do you have a
fix in the works already?
feel free. I have no code laying around
|
michael2008
pushed a commit
to michael2008/coredns
that referenced
this pull request
Feb 9, 2021
* upstream/master: (289 commits) auto go mod tidy build(deps): bump github.com/miekg/dns from 1.1.37 to 1.1.38 (coredns#4459) build(deps): bump github.com/aws/aws-sdk-go from 1.37.1 to 1.37.6 (coredns#4458) build(deps): bump github.com/Azure/go-autorest/autorest/azure/auth (coredns#4457) plugin/transfer: only allow outgoing axfr over tcp (coredns#4452) plugin/rewrite: copy msg before rewritting (coredns#4443) plugin/acl: add the ability to filter records (coredns#4389) build(deps): bump github.com/miekg/dns from 1.1.35 to 1.1.37 (coredns#4442) build(deps): bump github.com/aws/aws-sdk-go from 1.36.31 to 1.37.1 (coredns#4441) Makefile.release: Replace manifest-tool with docker manifest (coredns#4421) copy rrs (coredns#4416) plugin/trace - Use compatible tag name for datadog (coredns#4408) auto make -f Makefile.doc plugin/forward Add rcode and rtype to request_duration_seconds metric (coredns#4391) modify the error url of DWF (coredns#4425) modify the error url of hualala (coredns#4426) Corrected detection of K8s minor version (coredns#4430) core: flag blacklisting not needed anymore (coredns#4420) auto go mod tidy build(deps): bump github.com/aws/aws-sdk-go from 1.36.28 to 1.36.31 (coredns#4424) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.