Skip to content

Commit 6198dab

Browse files
authored
Don't add results to finalResults once promise was notified (#13196)
Motivation: We need to ensure we release the results if we dont end up transfering the ownership. In some cases we could have ended up to adding more results to the List without be sure that these will ever be released at all. Modifications: Only add results as long as we did not notify yet. Otherwise release these. Result: No more memory leak possible even if promise is already completed
1 parent c12e34a commit 6198dab

File tree

1 file changed

+22
-10
lines changed

1 file changed

+22
-10
lines changed

resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -870,16 +870,22 @@ private void onExpectedResponse(
870870
completeEarly = isCompleteEarly(converted);
871871
}
872872

873-
// We want to ensure we do not have duplicates in finalResult as this may be unexpected.
874-
//
875-
// While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an
876-
// ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay
877-
// for the extra memory copy and allocations in this cases later on.
878-
if (finalResult == null) {
879-
finalResult = new ArrayList<T>(8);
880-
finalResult.add(converted);
881-
} else if (isDuplicateAllowed() || !finalResult.contains(converted)) {
882-
finalResult.add(converted);
873+
// Check if the promise was done already, and only if not add things to the finalResult. Otherwise lets
874+
// just release things after we cached it.
875+
if (!promise.isDone()) {
876+
// We want to ensure we do not have duplicates in finalResult as this may be unexpected.
877+
//
878+
// While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an
879+
// ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay
880+
// for the extra memory copy and allocations in this cases later on.
881+
if (finalResult == null) {
882+
finalResult = new ArrayList<T>(8);
883+
finalResult.add(converted);
884+
} else if (isDuplicateAllowed() || !finalResult.contains(converted)) {
885+
finalResult.add(converted);
886+
} else {
887+
shouldRelease = true;
888+
}
883889
} else {
884890
shouldRelease = true;
885891
}
@@ -1047,11 +1053,17 @@ private void finishResolve(Promise<List<T>> promise, Throwable cause) {
10471053
if (!promise.isDone()) {
10481054
// Found at least one resolved record.
10491055
final List<T> result = filterResults(finalResult);
1056+
// Lets replace the previous stored result.
1057+
finalResult = Collections.emptyList();
10501058
if (!DnsNameResolver.trySuccess(promise, result)) {
10511059
for (T item : result) {
10521060
ReferenceCountUtil.safeRelease(item);
10531061
}
10541062
}
1063+
} else {
1064+
// This should always be the case as we replaced the list once notify the promise with an empty one
1065+
// and never add to it again.
1066+
assert finalResult.isEmpty();
10551067
}
10561068
return;
10571069
}

0 commit comments

Comments
 (0)