andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Remove id from DnsQueryContextManager whenever the promise is fullfilled.

Browse Source 
Motivation:

We did not handle the case when the query was cancelled which could lead to an exhausted id space. Beside this we did not not cancel the timeout on failed promises.

Modifications:

- Do the removal of the id from the manager in a FutureListener so its handled in all cases.
- Cancel the timeout whenever the original promise was full-filled.

Result:

Fixes https://github.com/netty/netty/issues/8013.
This commit is contained in:
Norman Maurer 2018-06-26 07:36:23 +02:00
parent f164759ea3
commit a214f2eb96
1 changed files with 26 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
resolver-dns/src/main/java/io/netty/resolver/dns/DnsQueryContext.java
View File

@ -28,6 +28,7 @@ import io.netty.handler.codec.dns.DnsRecord;
import io.netty.handler.codec.dns.DnsResponse; import io.netty.handler.codec.dns.DnsResponse;
import io.netty.handler.codec.dns.DnsSection; import io.netty.handler.codec.dns.DnsSection;
import io.netty.util.concurrent.Future; import io.netty.util.concurrent.Future;
import io.netty.util.concurrent.FutureListener;
import io.netty.util.concurrent.GenericFutureListener; import io.netty.util.concurrent.GenericFutureListener;
import io.netty.util.concurrent.Promise; import io.netty.util.concurrent.Promise;
import io.netty.util.concurrent.ScheduledFuture; import io.netty.util.concurrent.ScheduledFuture;
@ -39,7 +40,7 @@ import java.util.concurrent.TimeUnit;
import static io.netty.util.internal.ObjectUtil.checkNotNull; import static io.netty.util.internal.ObjectUtil.checkNotNull;
final class DnsQueryContext { final class DnsQueryContext implements FutureListener<AddressedEnvelope<DnsResponse, InetSocketAddress>> {
private static final InternalLogger logger = InternalLoggerFactory.getInstance(DnsQueryContext.class); private static final InternalLogger logger = InternalLoggerFactory.getInstance(DnsQueryContext.class);
@ -68,6 +69,9 @@ final class DnsQueryContext {
recursionDesired = parent.isRecursionDesired(); recursionDesired = parent.isRecursionDesired();
id = parent.queryContextManager.add(this); id = parent.queryContextManager.add(this);
// Ensure we remove the id from the QueryContextManager once the query completes.
promise.addListener(this);
if (parent.isOptResourceEnabled()) { if (parent.isOptResourceEnabled()) {
optResource = new AbstractDnsOptPseudoRrRecord(parent.maxPayloadSize(), 0, 0) { optResource = new AbstractDnsOptPseudoRrRecord(parent.maxPayloadSize(), 0, 0) {
// We may want to remove this in the future and let the user just specify the opt record in the query. // We may want to remove this in the future and let the user just specify the opt record in the query.
@ -119,9 +123,6 @@ final class DnsQueryContext {
if (future.isSuccess()) { if (future.isSuccess()) {
writeQuery(query, writePromise); writeQuery(query, writePromise);
} else { } else {
// Remove the id from the manager as we fail the query.
parent.queryContextManager.remove(nameServerAddr(), id);
Throwable cause = future.cause(); Throwable cause = future.cause();
promise.tryFailure(cause); promise.tryFailure(cause);
writePromise.setFailure(cause); writePromise.setFailure(cause);
@ -138,7 +139,7 @@ final class DnsQueryContext {
} else { } else {
writeFuture.addListener(new ChannelFutureListener() { writeFuture.addListener(new ChannelFutureListener() {
@Override @Override
public void operationComplete(ChannelFuture future) throws Exception { public void operationComplete(ChannelFuture future) {
onQueryWriteCompletion(writeFuture); onQueryWriteCompletion(writeFuture);
} }
}); });
@ -184,29 +185,18 @@ final class DnsQueryContext {
} }
private void setSuccess(AddressedEnvelope<? extends DnsResponse, InetSocketAddress> envelope) { private void setSuccess(AddressedEnvelope<? extends DnsResponse, InetSocketAddress> envelope) {
parent.queryContextManager.remove(nameServerAddr(), id);
// Cancel the timeout task.
final ScheduledFuture<?> timeoutFuture = this.timeoutFuture;
if (timeoutFuture != null) {
timeoutFuture.cancel(false);
}
Promise<AddressedEnvelope<DnsResponse, InetSocketAddress>> promise = this.promise; Promise<AddressedEnvelope<DnsResponse, InetSocketAddress>> promise = this.promise;
if (promise.setUncancellable()) { @SuppressWarnings("unchecked")
@SuppressWarnings("unchecked") AddressedEnvelope<DnsResponse, InetSocketAddress> castResponse =
AddressedEnvelope<DnsResponse, InetSocketAddress> castResponse = (AddressedEnvelope<DnsResponse, InetSocketAddress>) envelope.retain();
(AddressedEnvelope<DnsResponse, InetSocketAddress>) envelope.retain(); if (!promise.trySuccess(castResponse)) {
if (!promise.trySuccess(castResponse)) { // We failed to notify the promise as it was failed before, thus we need to release the envelope
// We failed to notify the promise as it was failed before, thus we need to release the envelope envelope.release();
envelope.release();
}
} }
} }
private void setFailure(String message, Throwable cause) { private void setFailure(String message, Throwable cause) {
final InetSocketAddress nameServerAddr = nameServerAddr(); final InetSocketAddress nameServerAddr = nameServerAddr();
parent.queryContextManager.remove(nameServerAddr, id);
final StringBuilder buf = new StringBuilder(message.length() + 64); final StringBuilder buf = new StringBuilder(message.length() + 64);
buf.append('[') buf.append('[')
@ -225,4 +215,18 @@ final class DnsQueryContext {
} }
promise.tryFailure(e); promise.tryFailure(e);
} }
@Override
public void operationComplete(Future<AddressedEnvelope<DnsResponse, InetSocketAddress>> future) {
// Cancel the timeout task.
final ScheduledFuture<?> timeoutFuture = this.timeoutFuture;
if (timeoutFuture != null) {
this.timeoutFuture = null;
timeoutFuture.cancel(false);
}
// Remove the id from the manager as soon as the query completes. This may be because of success, failure or
// cancellation
parent.queryContextManager.remove(nameServerAddr, id);
}
} }