Skip to content

Commit

Permalink
Don't add results to finalResults once promise was notified (#13196)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
normanmaurer authored Feb 7, 2023
1 parent c12e34a commit 6198dab
Showing 1 changed file with 22 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -870,16 +870,22 @@ private void onExpectedResponse(
completeEarly = isCompleteEarly(converted);
}

// We want to ensure we do not have duplicates in finalResult as this may be unexpected.
//
// While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an
// ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay
// for the extra memory copy and allocations in this cases later on.
if (finalResult == null) {
finalResult = new ArrayList<T>(8);
finalResult.add(converted);
} else if (isDuplicateAllowed() || !finalResult.contains(converted)) {
finalResult.add(converted);
// Check if the promise was done already, and only if not add things to the finalResult. Otherwise lets
// just release things after we cached it.
if (!promise.isDone()) {
// We want to ensure we do not have duplicates in finalResult as this may be unexpected.
//
// While using a LinkedHashSet or HashSet may sound like the perfect fit for this we will use an
// ArrayList here as duplicates should be found quite unfrequently in the wild and we dont want to pay
// for the extra memory copy and allocations in this cases later on.
if (finalResult == null) {
finalResult = new ArrayList<T>(8);
finalResult.add(converted);
} else if (isDuplicateAllowed() || !finalResult.contains(converted)) {
finalResult.add(converted);
} else {
shouldRelease = true;
}
} else {
shouldRelease = true;
}
Expand Down Expand Up @@ -1047,11 +1053,17 @@ private void finishResolve(Promise<List<T>> promise, Throwable cause) {
if (!promise.isDone()) {
// Found at least one resolved record.
final List<T> result = filterResults(finalResult);
// Lets replace the previous stored result.
finalResult = Collections.emptyList();
if (!DnsNameResolver.trySuccess(promise, result)) {
for (T item : result) {
ReferenceCountUtil.safeRelease(item);
}
}
} else {
// This should always be the case as we replaced the list once notify the promise with an empty one
// and never add to it again.
assert finalResult.isEmpty();
}
return;
}
Expand Down

0 comments on commit 6198dab

Please sign in to comment.