Dns Codec updated pull request #1622

Closed
wants to merge 29 commits into
from

Conversation

Projects
None yet
7 participants

mbakkar commented Jul 20, 2013

I had already closed the other pull request and deleted the repository. I don't think I can reopen pull requests. But here is an updated one (with some changes to work with 4.0.2).

I noticed when writing to a channel information isn't flushed automatically, I have to use writeAndFlush(). Intentional?

@ghost

ghost commented Jul 20, 2013

Build result for #1622 at ef5c715: Failure

Owner

normanmaurer commented Jul 20, 2013

Yes... See the release notes of 4.0.0.Final.

Will check later

Am 20.07.2013 um 18:59 schrieb mbakkar notifications@github.com:

I had already closed the other pull request and deleted the repository. I don't think I can reopen pull requests. But here is an updated one (with some changes to work with 4.0.2).

I noticed when writing to a channel information isn't flushed automatically, I have to use writeAndFlush(). Intentional?

You can merge this Pull Request by running

git pull https://github.com/mbakkar/netty-1 master
Or view, comment on, or merge it at:

#1622

Commit Summary

Updated for netty 4.0.2
File Changes

A codec/src/main/java/io/netty/handler/codec/dns/DnsEntry.java (381)
A codec/src/main/java/io/netty/handler/codec/dns/DnsHeader.java (175)
A codec/src/main/java/io/netty/handler/codec/dns/DnsMessage.java (132)
A codec/src/main/java/io/netty/handler/codec/dns/DnsQuery.java (31)
A codec/src/main/java/io/netty/handler/codec/dns/DnsQueryEncoder.java (109)
A codec/src/main/java/io/netty/handler/codec/dns/DnsQueryHeader.java (65)
A codec/src/main/java/io/netty/handler/codec/dns/DnsResponse.java (87)
A codec/src/main/java/io/netty/handler/codec/dns/DnsResponseDecoder.java (211)
A codec/src/main/java/io/netty/handler/codec/dns/DnsResponseHeader.java (242)
A codec/src/main/java/io/netty/handler/codec/dns/Question.java (71)
A codec/src/main/java/io/netty/handler/codec/dns/Resource.java (128)
A codec/src/main/java/io/netty/handler/codec/dns/ResponseCode.java (125)
A codec/src/main/java/io/netty/handler/codec/dns/package-info.java (21)
A codec/src/test/java/io/netty/handler/codec/dns/DnsCodecTest.java (105)
A codec/src/test/java/io/netty/handler/codec/dns/package-info.java (21)
A handler/src/main/java/io/netty/handler/dns/DnsCallback.java (220)
A handler/src/main/java/io/netty/handler/dns/DnsClientInitializer.java (37)
A handler/src/main/java/io/netty/handler/dns/DnsExchangeFactory.java (494)
A handler/src/main/java/io/netty/handler/dns/InboundDnsMessageHandler.java (53)
A handler/src/main/java/io/netty/handler/dns/ResourceCache.java (172)
A handler/src/main/java/io/netty/handler/dns/SingleResultCallback.java (59)
A handler/src/main/java/io/netty/handler/dns/decoder/AddressDecoder.java (64)
A handler/src/main/java/io/netty/handler/dns/decoder/DomainDecoder.java (42)
A handler/src/main/java/io/netty/handler/dns/decoder/MailExchangerDecoder.java (47)
A handler/src/main/java/io/netty/handler/dns/decoder/RecordDecoder.java (45)
A handler/src/main/java/io/netty/handler/dns/decoder/RecordDecoderFactory.java (135)
A handler/src/main/java/io/netty/handler/dns/decoder/ServiceDecoder.java (49)
A handler/src/main/java/io/netty/handler/dns/decoder/StartOfAuthorityDecoder.java (52)
A handler/src/main/java/io/netty/handler/dns/decoder/TextDecoder.java (54)
A handler/src/main/java/io/netty/handler/dns/decoder/package-info.java (21)
A handler/src/main/java/io/netty/handler/dns/decoder/record/MailExchangerRecord.java (57)
A handler/src/main/java/io/netty/handler/dns/decoder/record/ServiceRecord.java (113)
A handler/src/main/java/io/netty/handler/dns/decoder/record/StartOfAuthorityRecord.java (123)
A handler/src/main/java/io/netty/handler/dns/decoder/record/package-info.java (21)
A handler/src/main/java/io/netty/handler/dns/package-info.java (21)
A handler/src/test/java/io/netty/handler/dns/DnsImplementationTest.java (85)
A handler/src/test/java/io/netty/handler/dns/package-info.java (21)
Patch Links:

https://github.com/netty/netty/pull/1622.patch
https://github.com/netty/netty/pull/1622.diff

We need to make sure @mbakkar signs a CLA too if that wasn't done already :)

Really looking forward to getting this in by the end of the summer :)

mbakkar commented Jul 20, 2013

I believe I did sign a CLA in the beginning

@ghost

ghost commented Jul 20, 2013

Build result for #1622 at f16cd9b: Success

Owner

normanmaurer commented Jul 20, 2013

@caniszczyk let me review this and merge in at the end...

normanmaurer was assigned Jul 20, 2013

@normanmaurer normanmaurer commented on an outdated diff Jul 20, 2013

...main/java/io/netty/handler/codec/dns/DnsResponse.java
+
+ private final ByteBuf rawPacket;
+ private final int originalIndex;
+
+ public DnsResponse(ByteBuf rawPacket) {
+ this.rawPacket = rawPacket;
+ this.originalIndex = rawPacket.readerIndex();
+ }
+
+ /**
+ * Returns the non-decoded DNS response packet, at its
+ * <strong>original</strong> {@code readerIndex}.
+ */
+ @Override
+ public ByteBuf content() {
+ return rawPacket.readerIndex(originalIndex);
@normanmaurer

normanmaurer Jul 20, 2013

Owner

Why reset the index here ? I think this should not be done as we not do this in other ByteBufHolder impls

@normanmaurer normanmaurer commented on an outdated diff Jul 20, 2013

...va/io/netty/handler/codec/dns/DnsResponseDecoder.java
+ * @param buf
+ * the byte buffer containing the DNS packet
+ * @return the domain name for an entry
+ */
+ public static String readName(ByteBuf buf) {
+ int position = -1;
+ StringBuilder name = new StringBuilder();
+ for (int len = buf.readUnsignedByte(); buf.isReadable() && len != 0; len = buf.readUnsignedByte()) {
+ boolean pointer = (len & 0xc0) == 0xc0;
+ if (pointer) {
+ if (position == -1) {
+ position = buf.readerIndex() + 1;
+ }
+ buf.readerIndex((len & 0x3f) << 8 | buf.readUnsignedByte());
+ } else {
+ name.append(buf.toString(buf.readerIndex(), len, Charset.forName("UTF-8"))).append(".");
@normanmaurer

normanmaurer Jul 20, 2013

Owner

Use CharsetUtil.UTF_8

@normanmaurer normanmaurer commented on an outdated diff Jul 20, 2013

...va/io/netty/handler/codec/dns/DnsResponseDecoder.java
+ }
+
+ /**
+ * Decodes a resource record, given a DNS packet in a byte buffer.
+ *
+ * @param buf
+ * the byte buffer containing the DNS packet
+ * @return a {@link Resource} record containing response data
+ */
+ public static Resource decodeResource(ByteBuf buf) {
+ String name = readName(buf);
+ int type = buf.readUnsignedShort();
+ int aClass = buf.readUnsignedShort();
+ long ttl = buf.readUnsignedInt();
+ int len = buf.readUnsignedShort();
+ ByteBuf resourceData = Unpooled.buffer(len);
@normanmaurer

normanmaurer Jul 20, 2013

Owner

Would be better do pass in the ByteBufAllocator and use it for allocate new buffer

@normanmaurer normanmaurer commented on an outdated diff Jul 20, 2013

.../java/io/netty/handler/codec/dns/DnsQueryEncoder.java
+ }
+
+ /**
+ * Encodes the information in a {@link Question} and writes it to the
+ * specified {@link ByteBuf}.
+ *
+ * @param question
+ * the question being encoded
+ * @param buf
+ * the buffer the encoded data should be written to
+ */
+ public static void encodeQuestion(Question question, ByteBuf buf) {
+ String[] parts = question.name().split("\\.");
+ for (int i = 0; i < parts.length; i++) {
+ buf.writeByte(parts[i].length());
+ buf.writeBytes(parts[i].getBytes());
@normanmaurer

normanmaurer Jul 20, 2013

Owner

Pass in Charset to getBytes(...)

@normanmaurer normanmaurer commented on an outdated diff Jul 20, 2013

...va/io/netty/handler/codec/dns/DnsResponseDecoder.java
+ *
+ * @param buf
+ * the byte buffer containing the DNS packet
+ * @param offset
+ * the position at which the name begins
+ * @return the domain name for an entry
+ */
+ public static String getName(ByteBuf buf, int offset) {
+ StringBuilder name = new StringBuilder();
+ for (int len = buf.getUnsignedByte(offset++); buf.writerIndex() > offset && len != 0; len = buf
+ .getUnsignedByte(offset++)) {
+ boolean pointer = (len & 0xc0) == 0xc0;
+ if (pointer) {
+ offset = (len & 0x3f) << 8 | buf.getUnsignedByte(offset++);
+ } else {
+ name.append(buf.toString(offset, len, Charset.forName("UTF-8"))).append(".");
@normanmaurer

normanmaurer Jul 20, 2013

Owner

Use CharsetUtil.UTF_8

@normanmaurer normanmaurer commented on an outdated diff Jul 20, 2013

...va/io/netty/handler/codec/dns/DnsResponseDecoder.java
+ * specialized message handler.
+ *
+ * @param ctx
+ * the {@link ChannelHandlerContext} this
+ * {@link DnsResponseDecoder} belongs to
+ * @param buf
+ * the message being decoded, a {@link DatagramPacket} containing
+ * a DNS packet
+ * @param out
+ * the {@link List} to which decoded messages should be
+ * added
+ * @throws Exception
+ */
+ @Override
+ protected void decode(ChannelHandlerContext ctx, DatagramPacket packet, List<Object> out) throws Exception {
+ out.add(decodeResponse(packet.content()).retain());
@normanmaurer

normanmaurer Jul 20, 2013

Owner

why retain ?

@mbakkar mbakkar -Replaced Charset.forName with CharsetUtil.UTF_8
-DnsResponse returns content with unmodified index
c47ac23
Owner

normanmaurer commented Jul 20, 2013

@mbakkar what I would like to see is to create a new "module" called codec-dns and merge everything you have currently split into codec and handler into it.

@mbakkar mbakkar - Moved dns to its own module "codec-dns"
- Decoding and encoding methods now pass ByteBufAllocator and Charset
2248a5f

mbakkar commented Jul 20, 2013

I have to retain it because the DnsResponse is used in the next handler in the chain (InboundDnsMessageHandler for the impl test, and for the codec test it is the "Handler" inner-class in DnsCodecTest)

EDIT: some package-infos got erased, re-committing

@ghost

ghost commented Jul 20, 2013

Build result for #1622 at 17e3bb7: Success

@normanmaurer normanmaurer commented on an outdated diff Jul 21, 2013

...ain/java/io/netty/handler/dns/DnsExchangeFactory.java
+ * Checks if a DNS server can actually be connected to and queried for
+ * information by running through a request. This is a
+ * <strong>blocking</strong> method.
+ *
+ * @param address
+ * the DNS server being checked
+ * @return {@code true} if the DNS server provides a valid response
+ */
+ public static boolean validAddress(byte[] address) {
+ try {
+ int id = obtainId();
+ Channel channel = channelForAddress(address);
+ Callable<List<ByteBuf>> callback = new DnsCallback<List<ByteBuf>>(-1, sendQuery(DnsEntry.TYPE_A,
+ "google.com", id, channel));
+ return callback.call() != null;
+ } catch (Exception e) {
@normanmaurer

normanmaurer Jul 21, 2013

Owner

Don't swallow exceptions... Please at least log it.

@normanmaurer normanmaurer commented on an outdated diff Jul 21, 2013

...o/netty/handler/dns/decoder/RecordDecoderFactory.java
+ * decoded
+ * @param resource
+ * the resource record being decoded
+ * @return the decoded resource record
+ */
+ @SuppressWarnings("unchecked")
+ public <T> T decode(int type, DnsResponse response, Resource resource) {
+ RecordDecoder<?> decoder = decoders.get(type);
+ if (decoder == null) {
+ throw new IllegalStateException("Unsupported resource record type [id: " + type + "].");
+ }
+ T result = null;
+ try {
+ result = (T) decoder.decode(response, resource);
+ } catch (Exception e) {
+ e.printStackTrace();
@normanmaurer

normanmaurer Jul 21, 2013

Owner

Wouldn't it be better to throw a DecoderException here ?

@normanmaurer normanmaurer commented on an outdated diff Jul 21, 2013

...s/src/main/java/io/netty/handler/dns/DnsCallback.java
+ */
+ private void nextDns() {
+ if (serverIndex == -1) {
+ result = null;
+ } else {
+ byte[] dnsServerAddress = DnsExchangeFactory.getDnsServer(++serverIndex);
+ if (dnsServerAddress == null) {
+ result = null;
+ } else {
+ try {
+ Channel channel = DnsExchangeFactory.channelForAddress(dnsServerAddress);
+ for (int i = 0; i < queries.length; i++) {
+ channel.write(queries[i]).sync();
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
@normanmaurer

normanmaurer Jul 21, 2013

Owner

Log the exception

@normanmaurer normanmaurer commented on an outdated diff Jul 21, 2013

...ain/java/io/netty/handler/dns/DnsExchangeFactory.java
+ /**
+ * Removes an inactive channel after timing out. Internal use only.
+ *
+ * @param channel
+ * the channel to be removed
+ */
+ protected static void removeChannel(Channel channel) {
+ synchronized (dnsServerChannels) {
+ for (Iterator<Map.Entry<byte[], Channel>> iter = dnsServerChannels.entrySet().iterator(); iter.hasNext();) {
+ Map.Entry<byte[], Channel> entry = iter.next();
+ if (entry.getValue() == channel) {
+ if (channel.isOpen()) {
+ try {
+ channel.close().sync();
+ } catch (Exception e) {
+ e.printStackTrace();
@normanmaurer

normanmaurer Jul 21, 2013

Owner

Log the exception

@normanmaurer normanmaurer commented on an outdated diff Jul 21, 2013

...va/io/netty/handler/dns/InboundDnsMessageHandler.java
+ * the {@link DnsCallback} class to be linked to their callback.
+ */
+public class InboundDnsMessageHandler extends SimpleChannelInboundHandler<DnsResponse> {
+
+ /**
+ * When a {@link ReadTimeoutException} is caught, this means a DNS client
+ * has been active for a period of 30 seconds, and will be removed from the
+ * list of active channels.
+ */
+ @Override
+ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+ if (cause instanceof ReadTimeoutException) {
+ DnsExchangeFactory.removeChannel(ctx.channel());
+ return;
+ }
+ cause.printStackTrace();
@normanmaurer

normanmaurer Jul 21, 2013

Owner

forward to the next handler via ctx.fireExceptionCaught(cause)

@mbakkar mbakkar - Added more logging, removed printing of exceptions
- RecordDecoderFactory throws DecoderException now in constructor
0e4bb81
@ghost

ghost commented Jul 21, 2013

Build result for #1622 at 0e4bb81: Success

Check for logger.isWarnEnabled(...) before construct StringBuilder etc.

Check for logger.isErrorEnabled(...) before construct StringBuilder etc.

Check for logger.isErrorEnabled(...) before construct StringBuilder etc.

I think IllegalStateException was the right exception here

Owner

normanmaurer commented Jul 21, 2013

@trustin please also review..

@ghost

ghost commented Jul 21, 2013

Build result for #1622 at 89377aa: Success

Member

lw346 commented Jul 22, 2013

The unit tests seem to depend on 8.8.8.8 being accessible while the unit test is running, as well as the host-configured DNS server. Personally, I'm not a fan of unit tests that are dependent on external services. Thoughts?

Owner

normanmaurer commented Jul 22, 2013

@mbakkar @lw346 agree... would be better to mock this somehow..

mbakkar commented Jul 22, 2013

Well, I could use the system's DNS settings to get the server instead of hardcoding it. Or just have the server passed as a program argument. I don't see how it would be particularly helpful if the resolver isn't communicating with a real server (so another solution would be to write my own server which would be overkill, or write a fake server of sorts that sends predetermined data but I don't think that would be helpful).

Contributor

bk1te commented Jul 23, 2013

Testing codecs is all about seeing how they would behave in different situations. I think that common practice in netty is in using embeddable channel(correct me if i am wrong), and here I see two possible variants:

  • implement server side codecs
  • tcpdump all kind of requests that codec should support and stub it in tests

mbakkar commented Jul 24, 2013

I'll use EmbeddedChannel to confirm data is sent correctly and then write valid response packets (that I'll store in the program) to an EmbeddedChannel to confirm packets can be decoded.

Owner

normanmaurer commented Jul 24, 2013

@mbakkar +1 ... It's prefered to have tests working without the need of an internet connection or depending on other resources.

normanmaurer reopened this Jul 24, 2013

@ghost

ghost commented Jul 27, 2013

Build result for #1622 at 4bd031c: Success

@trustin trustin commented on an outdated diff Jul 29, 2013

...ain/java/io/netty/handler/codec/dns/ResponseCode.java
+ * ID 14, bad timestamp
+ */
+ BADTIME(14, "bad timestamp");
+
+ private final int errorCode;
+ private final String message;
+
+ /**
+ * Returns a formated message for an error received given an ID, or unknown
+ * if the error is unsupported.
+ *
+ * @param responseCode
+ * the error code's id
+ * @return formatted error message
+ */
+ public static String obtainError(int responseCode) {
@trustin

trustin Jul 29, 2013

Owner

This method could be split into two:

  • public static ResponseCode valueOf(int responseCode)
  • public String toString() which actually converts a ResponseCode to a string

If a user wants to get the name of an enum, he or she should use name() method.

I'd also add a method that returns the code of the enum e.g. public int code()

Owner

trustin commented Jul 29, 2013

Small things:

  • Is there any reason why some classes do not start with Dns? Particularly Resource, Question, and ResponseCode.
  • io.netty.handler.dns looks like it actually could be split into another module like netty-dns and moved to io.netty.dns because it is primarily meant to provide asynchronous DNS lookup functionality via DnsExchangeFactory. What do you think?
  • I wonder if some methods and classes can become package private rather than becoming public. For example, DnsQueryEncoder and DnsResponseDecoder have many public static methods. If possible, I'd like to hide as many classes from users, so that we have more room for future changes.

Big things:

I think a user will want to:

  • specify a EventLoop to perform a query to minimize latency between DNS queries and actual connection attempt,
  • apply different caching strategy for different cases,
  • find the simpliest way to get a single 'resolved' address among multiple ones and employ different strategy for choosing one of them (e.g. round robin, random, etc)

To address these needs, we could allow a user to instantiate different DNS resolver (say it's DnsResolver) specifying his or her favorite EventLoop, DNS cache, and single-record selection strategy, of course with sensible defaults. (.. rather than providing DnsExchangeFactory)

Bigger things:

Eventually, netty-transport could host an interface called Resolver so that a user can add netty-dns module to the class path and configure the alternative resolver when configuring a Bootstrap:

Bootstrap b = new Bootstrap();
b.resolver(new io.netty.dns.AsynchronousResolver(...)); // Default: JdkResolver
b.connect(new InetSocketAddress("www.google.com", 80));
@mbakkar mbakkar - Privatized/protected some static methods
- Renamed some classes to start with "Dns"
- New module "netty-dns" for impl of "codec-dns"
2f6297c

mbakkar commented Jul 29, 2013

I agree with all the small changes and I've implemented them, but should I rename all the classes decoder classes and record classes, i.e. "DnsTextDecoder" or "DnsAddressDecoder?"

Also I can't make the getName() and readName() methods of DnsResponseDecoder private since they are used in the impl to read names from resource records.

mbakkar commented Jul 29, 2013

So you want me to replace DnsExchangeFactory with DnsResolver (a non-static class) and add a few default classes that extend DnsResolver and make it possible to do those customizations (custom EventLoop, different caching strategies, etc)

@ghost

ghost commented Jul 29, 2013

Build result for #1622 at 2f6297c: Success

Owner

normanmaurer commented Jul 29, 2013

@trustin I like the idea with Resolver. Would be neat :) Also I agree we should try to hide as much as possible otherwise we may be in trouble later if we want to do some improvements.

Owner

trustin commented Jul 31, 2013

@mbakkar Yes. :-)

mbakkar commented Aug 5, 2013

Sorry, I've been busy with school nearing. I'll get to these things soon

@mbakkar mbakkar - New DnsResolver class
- Can customize selection strategy for single results
- Can customize caching strategy for resolver
- Can use custom EventLoop for resolver
9f10e51

mbakkar commented Aug 5, 2013

Ok, still in progress. The only caching strategy is the original one I had right now. I added round robin and random selection strategies.

@ghost

ghost commented Aug 5, 2013

Build result for #1622 at 9f10e51: Success

@ghost

ghost commented Aug 7, 2013

Build result for #1622 at feea47d: Success

Owner

trustin commented Aug 7, 2013

Please feel free to ping me once it's ready for a review.

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ return idx = idx + 1 & 0xffff;
+ }
+ }
+
+ private final List<byte[]> dnsServers = new ArrayList<byte[]>();
+ private final Map<byte[], Channel> dnsServerChannels = new HashMap<byte[], Channel>();
+
+ private final EventLoop eventLoop;
+ private final DnsCachingStrategy cache;
+ private final DnsSelectionStrategy selector;
+
+ public DnsResolver() {
+ this(null, new DnsDefaultCache(), new DnsRoundRobinSelectionStrategy());
+ }
+
+ public DnsResolver(EventLoop eventLoop, DnsCachingStrategy cache, DnsSelectionStrategy selector) {
@normanmaurer

normanmaurer Aug 7, 2013

Owner

I think it would be better to let the user specify the DNS servers to use and ditch the init() method

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ dnsServerChannels.put(dnsServerAddress, channel = b.connect().sync().channel());
+ }
+ return channel;
+ }
+ }
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List}
+ * either an IPv4 or IPv6 address for the specified domain.
+ *
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public <T> Future<T> lookup(String domain) throws UnknownHostException, SocketException, InterruptedException {
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ return resolveSingle(domain, dnsServers.get(0), DnsEntry.TYPE_A, DnsEntry.TYPE_AAAA);
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List} of
+ * either an IPv4 or IPv6 address for the specified domain, depending on the
+ * {@code family}.
+ *
+ * @param family
+ * {@code 4} for IPv4 addresses, {@code 6} for IPv6 addresses, or
+ * {@code null} for both
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public <T> Future<List<T>> lookup(String domain, Integer family) throws UnknownHostException, SocketException,
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ * {@code types}.
+ *
+ * @param domain
+ * the domain name being queried
+ * @param dnsServerAddress
+ * the DNS server to use
+ * @param types
+ * the desired resource record (only <strong>one</strong> type
+ * and one record can be returned in a single method call. The
+ * first valid resource record in the array of types will be
+ * returned)
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public <T> Future<T> resolveSingle(String domain, byte[] dnsServerAddress, int... types)
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ * {@code types}.
+ *
+ * @param domain
+ * the domain name being queried
+ * @param dnsServerAddress
+ * the DNS server to use
+ * @param types
+ * the desired resource records (only <strong>one</strong> type
+ * can be returned, but multiple resource records, in a single
+ * method call. The first valid type of resource records will be
+ * returned in a {@link List})
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public <T extends List<?>> Future<T> resolve(String domain, byte[] dnsServerAddress, int... types)
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ * @throws InterruptedException
+ */
+ public Future<List<ByteBuf>> resolve4(String domain) throws UnknownHostException, SocketException,
+ InterruptedException {
+ return resolve(domain, dnsServers.get(0), DnsEntry.TYPE_A);
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List} of
+ * IPv6 addresses as {@link ByteBuf}s.
+ *
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public Future<List<ByteBuf>> resolve6(String domain) throws UnknownHostException, SocketException,
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer

normanmaurer Aug 7, 2013

Owner

Why not return an Inet6Address in the List ?

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ * @throws InterruptedException
+ */
+ public Future<List<List<String>>> resolveTxt(String domain) throws UnknownHostException, SocketException,
+ InterruptedException {
+ return resolve(domain, dnsServers.get(0), DnsEntry.TYPE_TXT);
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List} of
+ * canonical name records as {@link String}s.
+ *
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public Future<List<String>> resolveCname(String domain) throws UnknownHostException, SocketException,
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ * @throws InterruptedException
+ */
+ public Future<List<String>> resolveCname(String domain) throws UnknownHostException, SocketException,
+ InterruptedException {
+ return resolve(domain, dnsServers.get(0), DnsEntry.TYPE_CNAME);
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List} of
+ * name server records as {@link String}s.
+ *
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public Future<List<String>> resolveNs(String domain) throws UnknownHostException, SocketException,
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ InterruptedException {
+ return resolve(domain, dnsServers.get(0), DnsEntry.TYPE_NS);
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List} of
+ * domain names as {@link String}s when given their corresponding IP
+ * address.
+ *
+ * @param ipAddress
+ * the ip address to perform a reverse lookup on
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public Future<List<String>> reverse(byte[] ipAddress) throws UnknownHostException, SocketException,
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ buf.release();
+ return future;
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List} of
+ * domain names as {@link String}s when given their corresponding IP
+ * address.
+ *
+ * @param ipAddress
+ * the ip address to perform a reverse lookup on
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public Future<List<String>> reverse(ByteBuf ipAddress) throws UnknownHostException, SocketException,
@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer

normanmaurer Aug 7, 2013

Owner

Why not pass a String in ?

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ for (int i = 0; i < types.length; i++) {
+ queries[i] = sendQuery(types[i], domain, id, channel);
+ }
+ return (eventLoop == null ? executor : eventLoop).submit(new DnsCallback<T>(this, dnsServers
+ .indexOf(dnsServerAddress), queries));
+ }
+
+ /**
+ * Returns a {@link Future} which can be used to obtain a {@link List} of
+ * IPv4 addresses as {@link ByteBuf}s.
+ *
+ * @throws UnknownHostException
+ * @throws SocketException
+ * @throws InterruptedException
+ */
+ public Future<List<ByteBuf>> resolve4(String domain) throws UnknownHostException, SocketException,
@normanmaurer

normanmaurer Aug 7, 2013

Owner

Why not return an Inet4Address in the list ?

@normanmaurer

normanmaurer Aug 7, 2013

Owner

This method should not throw any exceptions but notify the future about failures

@normanmaurer normanmaurer commented on an outdated diff Aug 7, 2013

netty-dns/src/main/java/io/netty/dns/DnsResolver.java
+ *
+ * @param type
+ * the type for the {@link DnsQuery}
+ * @param domain
+ * the domain being queried
+ * @param id
+ * the id for the {@link DnsQuery}
+ * @param channel
+ * the channel the {@link DnsQuery} is written to
+ * @return the {@link DnsQuery} being written
+ * @throws InterruptedException
+ */
+ private DnsQuery sendQuery(int type, String domain, int id, Channel channel) throws InterruptedException {
+ DnsQuery query = new DnsQuery(id);
+ query.addQuestion(new DnsQuestion(domain, type));
+ channel.writeAndFlush(query).sync();
@normanmaurer

normanmaurer Aug 7, 2013

Owner

Using sync here does not sound right... we not want to block!

Contributor

bk1te commented Aug 7, 2013

BTW why for specifying addresses byte array (byte[]) is used. what about String or InetAddress?

mbakkar commented Aug 8, 2013

Alright, I'll let the user specify all DNS servers with an option to use system's default. And I'll add some more options for reverse query arguments, and use InetAddress. I can do these changes tomorrow.

@ghost

ghost commented Aug 8, 2013

Build result for #1622 at c44ddf6: Success

mbakkar added some commits Aug 8, 2013

@mbakkar mbakkar - Many methods in DnsResolver no longer throw exceptions
- Strings can now be passed as IP addresses for reverse method in
DnsResolver
c5eb0f0
@mbakkar mbakkar - No blocking on writing queries aeb5a88
@ghost

ghost commented Aug 8, 2013

Build result for #1622 at c5eb0f0: Success

@ghost

ghost commented Aug 8, 2013

Build result for #1622 at aeb5a88: Success

mbakkar commented Aug 9, 2013

Bootstrap b = new Bootstrap();
b.resolver(new io.netty.dns.AsynchronousResolver(...)); // Default: JdkResolver
b.connect(new InetSocketAddress("www.google.com", 80));

@trustin I suppose InetSocketAddress would be our own class that extends java.net.InetSocketAddress since in Java's class the constructor looks up the host using their own resolver. And to ensure the JDK resolver isn't used automatically when it's not selected, Netty would have to only accept our own InetAddress classes? (I mean similarly to how Netty has it's own Future class and Executors to mirror Java's Future class and Executors)

@ghost

ghost commented Aug 9, 2013

Build result for #1622 at c4c490c: Success

Owner

trustin commented Aug 9, 2013

You are correct. It would be an overkill to re-implement everything JDK provides though. Perhaps we could introduce a new SocketAddress type, such as UnresolvedInetSocketAddress? (e.g. b.connect(new UnresolvedInetSocketAddress("www.google.com", 80));)

I wonder it will make any sense to introduce UnresolvedInetAddress. It obviously cannot extend InetAddress because its constructor is package-private, and is of dubious value in my opinion.

Owner

trustin commented Aug 9, 2013

Also, to avoid a user mistake, we could log at WARN level when an asynchronous resolver and InetSocketAddress are specified together. (only once)

Owner

normanmaurer commented Aug 9, 2013

@trustin we could also @deprecated the constructor that takes an InetSocketAddress.... I don't think introduce an UnresolvedInetSocketAddress is a good idea..

Owner

trustin commented Aug 9, 2013

If you don't think UnresolvedInetSocketAddress is a good idea, what would be your suggestion? I'd keep the methods that accept InetSocketAddress though because it's useful for simple use cases anyway.

Contributor

bk1te commented Aug 9, 2013

maybe use InetSocketAddress.createUnresolved when using with custom resolver?

Owner

trustin commented Aug 9, 2013

I didn't realize that there is such a method. Thanks @bk1te :-) Yeah, we should use InetSocketAddress.createUnresolved() rather than introducing something else.

Owner

normanmaurer commented Aug 9, 2013

Wow good catch! +1 for that

Am 09.08.2013 um 09:35 schrieb Trustin Lee notifications@github.com:

I didn't realize that there is such a method. Thanks @bk1te :-) Yeah, we should use InetSocketAddress.createUnresolved() rather than introducing something else.


Reply to this email directly or view it on GitHub.

Contributor

bk1te commented Aug 9, 2013

happy to help.

mbakkar commented Aug 9, 2013

Nice one! I should look closer at the APIs I'm working with :P

Alright, so I guess if a user passes an InetAddress, I'll print out a warning if the resolver being used isn't the default JDK one. Otherwise, if the asynchronous resolver is selected, I should instantly resolve the address and get it from the Future when connecting.

mbakkar commented Aug 10, 2013

Well, that was painful.

@ghost

ghost commented Aug 10, 2013

Build result for #1622 at a3beb6a: Success

mbakkar commented Aug 11, 2013

@trustin is there a reason Channel and Bootstrap and other classes widely use SocketAddress as opposed to InetSocketAddress? Does Netty have its own SocketAddress subclass, or can I safely assume all SocketAddress references are actually InetSocketAddress, and so I can cast them as such without checking?

Owner

normanmaurer commented Aug 11, 2013

@mbakkar it's because some "transports" are not network based and so not use an InetSocketAddress. check for example the EmbeddedChannel and LocalChannel.

mbakkar commented Aug 11, 2013

In progress... it seems to work, but I need to clean it up and replace thrown exceptions with logged warnings. And also edit ServerBootstrap

@ghost

ghost commented Aug 11, 2013

Build result for #1622 at 6ec8a63: Failure

@ghost

ghost commented Aug 11, 2013

Build result for #1622 at 18fe8e1: Failure

@ghost

ghost commented Aug 11, 2013

Build result for #1622 at 75b1d79: Failure

mbakkar commented Aug 11, 2013

I've gotta add pom.xml for the two new modules..

@ghost

ghost commented Aug 11, 2013

Build result for #1622 at 562767b: Failure

@ghost

ghost commented Aug 11, 2013

Build result for #1622 at 218a65d: Failure

@ghost

ghost commented Aug 11, 2013

Build result for #1622 at 6b8b35e: Failure

Could you explain why netty-transport has to depend on netty-dns?

mbakkar commented Aug 16, 2013

Doesn't it need to use its resolver in Bootstrap classes?

Owner

trustin commented Aug 16, 2013

Not really. The resolver interface should reside at netty-transport. netty-dns should provide only the async resolver implementation.

Owner

trustin commented Aug 16, 2013

I guess this could clear things a lot:

netty-transport:
public interface io.netty.dns.DomainNameResolver

netty-asyncdns:
public class io.netty.dns.async.AsyncDomainNameResolver implements io.netty.dns.DomainNameResolver

i.e. io.netty.dns belongs to netty-transport. io.netty.dns.async belongs to netty-asyncdns.

mbakkar commented Aug 16, 2013

I see, and AsyncDomainNameResolver would have an instance of/extend DnsResolver, good idea.

@normanmaurer normanmaurer commented on the diff Aug 19, 2013

...va/io/netty/handler/codec/dns/DnsResponseDecoder.java
+ /**
+ * Decodes a full DNS response packet.
+ *
+ * @param buf
+ * the raw DNS response packet
+ * @return the decoded {@link DnsResponse}
+ */
+ protected static DnsResponse decodeResponse(ByteBuf buf, ByteBufAllocator allocator) {
+ DnsResponse response = new DnsResponse(buf);
+ DnsResponseHeader header = decodeHeader(response, buf);
+ response.setHeader(header);
+ for (int i = 0; i < header.getReadQuestions(); i++) {
+ response.addQuestion(decodeQuestion(buf));
+ }
+ if (header.getResponseCode() != 0) {
+ System.err.println("Encountered error decoding DNS response for domain \""
@normanmaurer

normanmaurer Aug 19, 2013

Owner

I think you should just use "return response" here.

Contributor

travishaagen commented Aug 19, 2013

I see that DnsResource provides access to the TTL value, but the decoders in io.netty.dns.decoder do not expose the TTL. For some kinds of DNS clients, we might need the TTL, to provide a secondary caching layer (e.g., Sender Policy Framework).

If this is an uncommon need, then one could extend all of the decoders for their particular use-case, but if clients will often need to know the TTL, then perhaps it should be copied from DnsResource within the decode-function and exposed by the decoders.

@mbakkar how are things going with this project?

mbakkar commented Sep 4, 2013

Hi just a brief update.. I haven't really made progress on this in the past 2 weeks. I know clearly what I need to do but I've been stretched for time since I started school. I finish handing in a couple major assignments tomorrow at 11:59 PM, and then I should have a few days leeway where I can continue working on this after that. Sorry for getting back so late..!

Thanks @mbakkar! Pencils down starts next week so it would be good to start aiming for the finish line on the project.

@trustin is in the main Twitter office this week so we can hopefully work on getting the code in a state that it can be merged into master :)

Owner

trustin commented Sep 9, 2013

@mbakkar, no worries! You've done a great job so far. Please take your time.

@caniszczyk, sure. I'll review it and merge it into master once it's ready. :-)

Owner

normanmaurer commented Nov 5, 2013

@mbakkar @caniszczyk @trustin working on clean this up now and merge to master.. stay tuned.

Owner

normanmaurer commented Nov 5, 2013

I merged everything into dns branch.. Let me polish the code there. So closing this now.. @mbakkar thanks again for all your work

@normanmaurer normanmaurer added a commit that referenced this pull request Mar 10, 2014

@normanmaurer normanmaurer DNS codec for Netty which is based on the work of [#1622].
This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use
of the dns codec and netty transport itself.
e860b92

@normanmaurer normanmaurer added a commit that referenced this pull request Jun 2, 2014

@normanmaurer normanmaurer DNS codec for Netty which is based on the work of [#1622].
This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use
of the dns codec and netty transport itself.
28e4e71

@normanmaurer normanmaurer added a commit that referenced this pull request Jun 2, 2014

@normanmaurer normanmaurer DNS codec for Netty which is based on the work of [#1622].
Motivation:
As part of GSOC 2013 we had @mbakkar working on a DNS codec but did not integrate it yet as it needs some cleanup. This commit is based on @mbakkar's work and provide the codec for DNS.

Modifications:
Add DNS codec

Result:
Reusable DNS codec will be included in netty.

This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use
of the dns codec and netty transport itself.
fc2f0c7

@normanmaurer normanmaurer added a commit that referenced this pull request Jun 10, 2014

@normanmaurer normanmaurer DNS codec for Netty which is based on the work of [#1622].
Motivation:
As part of GSOC 2013 we had @mbakkar working on a DNS codec but did not integrate it yet as it needs some cleanup. This commit is based on @mbakkar's work and provide the codec for DNS.

Modifications:
Add DNS codec

Result:
Reusable DNS codec will be included in netty.

This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use
of the dns codec and netty transport itself.
805ba15

@normanmaurer normanmaurer added a commit that referenced this pull request Jun 10, 2014

@normanmaurer normanmaurer DNS codec for Netty which is based on the work of [#1622].
Motivation:
As part of GSOC 2013 we had @mbakkar working on a DNS codec but did not integrate it yet as it needs some cleanup. This commit is based on @mbakkar's work and provide the codec for DNS.

Modifications:
Add DNS codec

Result:
Reusable DNS codec will be included in netty.

This PR also includes a AsynchronousDnsResolver which allows to resolve DNS entries in a non blocking way by make use
of the dns codec and netty transport itself.
e3c76ec

jrudolph referenced this pull request in akka/akka Feb 10, 2015

Open

Add non-blocking DNS resolution #16846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment