Fix DnsNameResolver TCP fallback test and message leaks#9647
Fix DnsNameResolver TCP fallback test and message leaks#9647normanmaurer merged 4 commits intonetty:4.1from
Conversation
|
Can one of the admins verify this patch? |
|
The changes look bigger than they are due to some reordering and indentation changes, recommend viewing diff with |
resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java
Outdated
Show resolved
Hide resolved
|
@netty-bot test this please |
|
@njhill please rebase |
There was a problem hiding this comment.
nit: release first before log.
There was a problem hiding this comment.
I think we should preserve the logic and fallback to the original truncated response if we did not receive a TCP response just like we did before:
https://github.com/netty/netty/pull/9647/files#diff-6678a05ef56f2434e7aa20acbe9d97e9L1277
Also beside this we should not log the exception if we received the response before. As the exception may be just a "connection reset" and so harmless. Also consider calling ctx.close() in any case to ensure the channel is closed in all cases.
There was a problem hiding this comment.
Now fixed. I didn’t add an explicit close since the channel is closed immediately in the listener below.
There was a problem hiding this comment.
why not make this a real java doc ?
There was a problem hiding this comment.
lets rename this to trySuccess as this is what it does ;)
Motivation A memory leak related to DNS resolution was reported in netty#9634, specifically linked to the TCP retry fallback functionality that was introduced relatively recently. Upon inspection it's apparent that there are some error paths where the original UDP response might not be fully released, and more significantly the TCP response actually leaks every time on the fallback success path. It turns out that a bug in the unit test meant that the intended TCP fallback path was not actually exercised, so it did not expose the main leak in question. Modifications - Fix DnsNameResolverTest#testTruncated0 dummy server fallback logic to first read transaction id of retried query and use it in replayed response - Adjust semantic of internal DnsQueryContext#finish method to always take refcount ownership of passed in envelope - Reorder some logic in DnsResponseHandler fallback handling to verify the context of the response is expected, and ensure that the query response are either released or propagated in all cases. This also reduces a number of redundant retain/release pairings Result Fixes netty#9634
e1c0e3e to
fa84a02
Compare
|
@normanmaurer addressed comments, PTAL |
|
@netty-bot test this please |
|
@dziemba can you verify this one as well ? |
|
@normanmaurer I've built a local snapshot from this branch (fa84a02) and ran the reproduction script and some additional internal test suites we have. Looks all good now, no more leaks 🎉 Thanks a lot for the quick fix! |
|
@netty-bot test this please |
Motivation A memory leak related to DNS resolution was reported in #9634, specifically linked to the TCP retry fallback functionality that was introduced relatively recently. Upon inspection it's apparent that there are some error paths where the original UDP response might not be fully released, and more significantly the TCP response actually leaks every time on the fallback success path. It turns out that a bug in the unit test meant that the intended TCP fallback path was not actually exercised, so it did not expose the main leak in question. Modifications - Fix DnsNameResolverTest#testTruncated0 dummy server fallback logic to first read transaction id of retried query and use it in replayed response - Adjust semantic of internal DnsQueryContext#finish method to always take refcount ownership of passed in envelope - Reorder some logic in DnsResponseHandler fallback handling to verify the context of the response is expected, and ensure that the query response are either released or propagated in all cases. This also reduces a number of redundant retain/release pairings Result Fixes #9634
Motivation
A memory leak related to DNS resolution was reported in #9634, specifically linked to the TCP retry fallback functionality that was introduced relatively recently. Upon inspection it's apparent that there
are some error paths where the original UDP response might not be fully released, and more significantly the TCP response actually leaks every time on the fallback success path.
It turns out that a bug in the unit test meant that the intended TCP fallback path was not actually exercised, so it did not expose the main leak in question.
Modifications
DnsNameResolverTest#testTruncated0dummy server fallback logic to first read transaction id of retried query and use it in replayed responseDnsQueryContext#finishmethod to always take refcount ownership of passed in envelopeDnsResponseHandlerfallback handling to verify the context of the response is expected, and ensure that the query response are either released or propagated in all cases. This also reduces a number of redundant retain/release pairingsResult
Fixes #9634