Motivation:
It should be possible to build a DnsNameResolver with a null resolvedAddressTypes, defaulting then to DEFAULT_RESOLVE_ADDRESS_TYPES (see line 309).
Sadly, `preferredAddressType` is then called on line 377 with the original parameter instead of the instance attribute, causing an NPE when it's null.
Modification:
Call preferredAddressType with instance attribuet instead of constructor parameter.
Result:
No more NPE
Motivation:
ba594bcf4a added a utility to parse searchdomains defined in /etc/resolv.conf but did not correctly handle the case when multiple are defined that are seperated by either whitespace or tab.
Modifications:
- Correctly parse multiple entries
- Add unit test.
Result:
Correctly parse multiple searchdomain entries.
* Use AuthoritativeDnsServerCache for creating the new redirect stream.
Motivation:
At the moment if a user wants to provide custom sorting of the nameservers used for redirects it needs to be implemented in two places. This is more complicated as it needs to be.
Modifications:
- Just delegate to the AuthoritativeDnsServerCache always as we fill it before we call newRedirectDnsServerStream anyway.
Result:
Easier way for the user to implement custom sorting.
* Add cache for CNAME mappings resolved during lookup of DNS entries.
Motivation:
If the CNAMEd hostname is backed by load balancing component, typically the final A or AAAA DNS records have small TTL. However, the CNAME record itself is setup with longer TTL.
For example:
* x.netty.io could be CNAMEd to y.netty.io with TTL of 5 min
* A / AAAA records for y.netty.io has a TTL of 0.5 min
In current Netty implementation, original hostname is saved in resolved cached with the TTL of final A / AAAA records. When that cache entry expires, Netty recursive resolver sends at least two queries — 1st one to be resolved as CNAME record and the 2nd one to resolve the hostname in CNAME record.
If CNAME record was cached, only the 2nd query would be needed most of the time. 1st query would be needed less frequently.
Modifications:
Add a new CnameCache that will be used to cache CNAMEs and so may reduce queries.
Result:
Less queries needed when CNAME is used.
Motivation
Applications should not depend on internal packages with Java 9 and later. This cause a warning now, but will break in future versions of Java.
Modification
This change adds methods to UnixResolverDnsServerAddressStreamProvider (following after #6844) that parse /etc/resolv.conf for domain and search entries. Then DnsNameResolver does not need to rely on sun.net.dns.ResolverConfiguration to do this.
Result
Fixes#8318. Furthermore, at least in my testing with Java 11, this also makes multiple search entries work properly (previously I was only getting the first entry).
Motivation:
We should not try to cast the Channel to a DatagramChannel as this will cause a ClassCastException.
Modifications:
- Do not cast
- rethrow from constructor if we detect the registration failed.
- Add unit test.
Result:
Propagate correct exception.
Motiviation:
We incorrectly did ignore NS servers during redirect which had no ADDITIONAL record. This could at worse have the affect that we failed the query completely as none of the NS servers had a ADDITIONAL record. Beside this using a DnsCache to cache authoritative nameservers does not work in practise as we we need different features and semantics when cache these servers (for example we also want to cache unresolved nameservers and resolve these on the fly when needed).
Modifications:
- Correctly take NS records into account that have no matching ADDITIONAL record
- Correctly handle multiple ADDITIONAL records for the same NS record
- Introduce AuthoritativeDnsServerCache as a replacement of the DnsCache when caching authoritative nameservers + adding default implementation
- Add an adapter layer to reduce API breakage as much as possible
- Replace DnsNameResolver.uncachedRedirectDnsServerStream(...) with newRedirectDnsServerStream(...)
- Add unit tests
Result:
Our DnsResolver now correctly handle redirects in all cases.
Motivation:
We are currently always remove all entries from the cache for a hostname if the lowest TTL was reached but schedule one for each of the cached entries. This is wasteful.
Modifications:
- Reimplement logic to schedule TTL to only schedule a new removal task if the requested TTL was actual lower then the one for the already scheduled task.
- Ensure we only remove from the internal map if we did not replace the Entries in the meantime.
Result:
Less overhead in terms of scheduled tasks for the DefaultDnsCache
Motivation:
We should ensure we return the same cached entries for the hostname and hostname ending with dot. Beside this we also should use it for the searchdomains as well.
Modifications:
- Internally always use hostname with a dot as a key and so ensure we correctly handle it in the cache.
- Also query the cache for each searchdomain
- Add unit tests
Result:
Use the same cached entries for hostname with and without trailing dot. Query the cache for each searchdomain query as well
Motivation:
55fec94592 fixed a bug where we did not correctly clear all caches when the resolver was closed but did not add a testcase.
Modifications:
Add testcase.
Result:
More tests.
Motivation:
DnsNameResolver manages search domains and will retry the request with the different search domains provided to it. However if the query results in an invalid hostname, the Future corresponding to the resolve request will never be completed.
Modifications:
- If a resolve attempt results in an invalid hostname and the query isn't issued we should fail the associated promise
Result:
No more hang from DnsNameResolver if search domain results in invalid hostname.
Motivation:
At the moment we only clear the resolveCache when the Channel is closed. We should also do the same for the authoritativeDnsServerCache.
Modifications:
Add authoritativeDnsServerCache.clear() to the Channel closeFuture.
Result:
Correctly clear all caches.
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.
Motivation:
Whenever we fail the query we should also remove the id from the DnsQueryContextManager.
Modifications:
Remove the id from the DnsQueryContextManager if we fail the query because the channel failed to become active.
Result:
More correct code.
Motivation:
At the moment if you do a resolveAll and at least one A / AAAA record is present we will not follow any CNAMEs that are also present. This is different to how the JDK behaves.
Modifications:
- Allows follow CNAMEs.
- Add unit test.
Result:
Fixes https://github.com/netty/netty/issues/7915.
Motivation:
a598c3b69b added a upper limit for ttl but missed to also do the same for minTtl.
Modifications:
- Add upper limit for minTtl
- Add testcase.
Result:
No more IllegalArgumentException possible.
Motivation:
Due a bug we did never store more then one address per hostname in DefaultDnsCache.
Modifications:
- Correctly store multiple entries per hostname
- Add tests
Result:
DefaultDnsCache correctly stores more then one entry. Also fixes https://github.com/netty/netty/issues/7882 .
Motivation:
In b47fb81799 we limited the max supported delay to match what our internal implementat can support. Because of this it was possible that DefaultDnsCache produced an IllegalArgumentException when it tried to schedule a expiration > 3 years.
Modifications:
Limit the max supported TTL to 2 years which is safe for all our EventLoop implementations.
Result:
No more exceptions when adding records to the cache with a huge TTL.
Motivation:
Right now to customize DNS name resolver when using DnsAddressResolverGroup
one should subclass implementation and override newNameResolver method when
in fact it's possible to collect all settings in a DnsNameResolverBuilder
instance. Described in #7749.
Modifications:
- Added new constructor for DnsNameResolverBuilder in order to delay
EventLoop specification
- Added copy() method to DnsNameResolverBuilder to provide an immutable
copy of the builder
- Added new single-argument constructor for DnsAddressResolverGroup and
RoundRobinDnsAddressResolverGroup accepting DnsNameResolverBuilder
instance
- DnsAddressResolverGroup to build a new resolver using DnsNameResolverBuilder
given instead of creating a new one
- Test cases to check that changing channelFactory after the builder was passed
to create a DnsNameResolverGroup would not propagate to the name resolver
Result:
Much easier to customize DNS settings w/o subclassing DnsAddressResolverGroup
Motivation:
Currently, if a DNS server returns a non-preferred address type before the preferred one, then both will be returned as the result, and when only taking a single one, this usually ends up being the non-preferred type. However, the JDK requires lookups to only return the preferred type when possible to allow for backwards compatibility.
To allow a client to be able to resolve the appropriate address when running on a machine that does not support IPv6 but the DNS server returns IPv6 addresses before IPv4 addresses when querying.
Modification:
Filter the returned records to the expected type when both types are present.
Result:
Allows a client to run on a machine with IPv6 disabled even when a server returns both IPv4 and IPv6 results. Netty-based code can be a drop-in replacement for JDK-based code in such circumstances.
This PR filters results before returning them to respect JDK expectations.
* Add DnsNameResolver.resolveAll(DnsQuestion)
Motivation:
A user is currently expected to use DnsNameResolver.query() when he or
she wants to look up the full DNS records rather than just InetAddres.
However, query() only performs a single query. It does not handle
/etc/hosts file, redirection, CNAMEs or multiple name servers.
As a result, such a user has to duplicate all the logic in
DnsNameResolverContext.
Modifications:
- Refactor DnsNameResolverContext so that it can send queries for
arbitrary record types.
- Rename DnsNameResolverContext to DnsResolveContext
- Add DnsAddressResolveContext which extends DnsResolveContext for
A/AAAA lookup
- Add DnsRecordResolveContext which extends DnsResolveContext for
arbitrary lookup
- Add DnsNameResolverContext.resolveAll(DnsQuestion) and its variants
- Change DnsNameResolverContext.resolve() delegates the resolve request
to resolveAll() for simplicity
- Move the code that decodes A/AAAA record content to DnsAddressDecoder
Result:
- Fixes#7795
- A user does not have to duplicate DnsNameResolverContext in his or her
own code to implement the usual DNS resolver behavior.
Motivation:
When we do DNS queries we need to ensure we always release the AddressEnvelope.
Modifications:
Also release the AddressEnvelope if the original resolution was done in the meantime and we did not cancel the extra query yet.
Result:
Should fix [#7713]
Motivation:
When following a CNAME response DnsNameResovlerContext may issue a A and AAAA query. However the DnsNameResolverContext would have already issued a A and AAAA query to get the CNAME response, and this may result in 2 additional A/AAAA queries per CNAME response.
Modifications:
- DnsNameResovlerContext#followCname shouldn't issue 2 queries, but instead just a single query with the same record type as the original query
Result:
No more duplicate queries as a result of CNAME responses.
Motivation:
DnsServerAddressStream provides an iterator like interface but maybe expected to start at a specific point upon each new usage. If a DnsServerAddressStream is re-used in multiple independent iterations the order of iteration maybe incorrect. DnsNameResolverContext has a fallback DnsServerAddressStream reference if the cache doesn't contain a hit, but it is shared across multiple independent iterations. This may lead to undesirable DNS query order.
Modifications:
- DnsNameResolverContext#getNameServers should duplicate the default DnsServerAddressStream
Result:
Consistent iteration over the default DnsServerAddressStream in DnsNameResolverContext.
Motivation:
When following a CNAME it is possible there are multiple name servers to query against. However DnsNameResolverContext#followCname explicitly only uses the first name server address when attempting the query. This may lead to resolution failures because we didn't try all the available name servers.
Modifications:
DnsNameResolverContext#followCname should not just try the first name server, but it should try all name servers
Result:
More complete CNAME resolution.
Motivation:
At the moment DefaultDnsCache will expire each record dependong on its own TTL. This may result in unexpected results for the end-user especially if the user for example uses IPV4_PREFERED but the cached AAAA records has a higher TTL then the A records and so the A record was removed. In this case we would only return the AAAA record and not even try to refresh.
Modifications:
Always expire all records for a hostname when one TTL is reached.
Result:
Fixes [#7329]
Motivation:
The usage of DnsCache in DnsNameResolver was racy in general. First of the isEmpty() was not called in a synchronized block while we depended on synchronized. The other problem was that this whole synchronization only worked if the DefaultDnsCache was used and the returned List was not wrapped by the user.
Modifications:
- Rewrite DefaultDnsCache to not depend on synchronization on the returned List by using a CoW approach.
Result:
Fixes [#7583] and other races.
Motivation:
We need to ensure we only call List.* methods in the synchronized block as the returned List may not be thread-safe.
Modifications:
Do not call isEmpty() outside of the synchronized block.
Result:
Fixes [#7583]
Motivation:
DnsNameResolverTest has not been updated in a while.
Modifications:
- Update the DOMAINS definition in DnsNameResolverTest
Result:
More current domain names.
Automatic-Module-Name entry provides a stable JDK9 module name, when Netty is used in a modular JDK9 applications. More info: http://blog.joda.org/2017/05/java-se-9-jpms-automatic-modules.html
When Netty migrates to JDK9 in the future, the entry can be replaced by actual module-info descriptor.
Modification:
The POM-s are configured to put the correct module names to the manifest.
Result:
Fixes#7218.
Motivation:
At the moment there is not way for the user to know if resolving a domain was failed because the domain was unkown or because of an IO error / timeout. If it was caused by an timeout / IO error the user may want to retry the query. Also if the query was failed because of an IO error / timeout we should not cache it.
Modifications:
- Add DnsNameResolverTimeoutException and include it in the UnkownHostException if the domain could not be resolved because of an timeout. This will allow the user to retry the query when inspecting the cause.
- Do not cache IO errors / timeouts
- Add unit test
Result:
Easier for users to implement retries for DNS querys and not cache IO errors / timeouts.
Motivation:
At the moment there is not way for the user to know if resolving a domain was failed because the domain was unkown or because of an IO error / timeout. If it was caused by an timeout / IO error the user may want to retry the query. Also if the query was failed because of an IO error / timeout we should not cache it.
Modifications:
- Add DnsNameResolverTimeoutException and include it in the UnkownHostException if the domain could not be resolved because of an timeout. This will allow the user to retry the query when inspecting the cause.
- Do not cache IO errors / timeouts
- Add unit test
Result:
Easier for users to implement retries for DNS querys and not cache IO errors / timeouts.
Motivation:
Minor cleanup from 844d804 just to reduce the conditional statements and indentation level.
Modifications:
- combine the else + if into an else if statement
Result:
Code cleaned up.
Motivation:
DN resolution does not fall back to the "original name" lookup after search list is checked. This results in a failure to resolve any name (outside of search list) that has number of dots less than resolv.conf's ndots value (which, for example, is often the case in the context of Kubernetes where kubelet passes on resolv.conf containing "options ndots:5").
It also does not go through the search list in a situation described in resolv.conf man:
"The default for n[dots] is 1, meaning that if there are any dots in a name, the name will be tried first as an absolute name before any search list elements are appended to it."
Modifications:
DnsNameResolverContext::resolve was updated to match Go's https://github.com/golang/go/blob/release-branch.go1.9/src/net/dnsclient_unix.go#L338 logic.
Result:
DnsNameResolverContext::resolve will now try to resolve "original name" if search list yields no results when number of dots in the original name is less than resolv.conf's ndots value. It will also go through the search list in case "origin name" resolution fails and number of dots is equal or larger than resolv.conf's ndots value.
Motivation:
We should not try to use UnixResolverDnsServerAddressStreamProvider when on Windows as it will log some error that will produce noise and may confuse users.
Modifications:
Just use DefaultDnsServerAddressStreamProvider if windows is used.
Result:
Less noise in the logs. This was reported in vert.x: https://github.com/eclipse/vert.x/issues/2204
Motviation:
DnsNameResolverContext#followCname attempts to build a query to follow a CNAME, but puts the original hostname in the DnsQuery instead of the CNAME hostname. This will result in not following CNAME redirects correctly.
Result:
- DnsNameResolverContext#followCname should use the CNAME instead of the original hostname when building the DnsQuery
Result:
More correct handling of redirect queries.
Motivation:
Even if it's a super micro-optimization (most JVM could optimize such
cases in runtime), in theory (and according to some perf tests) it
may help a bit. It also makes a code more clear and allows you to
access such methods in the test scope directly, without instance of
the class.
Modifications:
Add 'static' modifier for all methods, where it possible. Mostly in
test scope.
Result:
Cleaner code with proper 'static' modifiers.
Motivation:
Without a 'serialVersionUID' field, any change to a class will make
previously serialized versions unreadable.
Modifications:
Add missed 'serialVersionUID' field for all Serializable
classes.
Result:
Proper deserialization of previously serialized objects.
Motivation:
DnsCache (an interface) is coupled to DnsCacheEntry (a final class). This means that DnsCache implementations can't implement their own DnsCacheEntry objects if the default behavior isn't appropriate.
Modifications:
- DnsCacheEntry should be moved to DefaultDnsCache as it is an implementation detail
- DnsCache#cache(..) should return a new DnsCacheEntry
- The methods which from DnsCacheEntry that were used outside the scope of DefaultDnsCache should be moved into an interface
Result:
DnsCache is more extensible and not tightly coupled to a default implementation of DnsCacheEntry.
Motivation:
To be able to build with latest java9 release we need to adjust commons-lang version and maven-enforcer-plugin.
Modifications:
- Use commons-lang 2.6.0
- Use maven-enforcer-plugin 3.0.0.M1 when building with java9
Result:
Netty builds again with latest java9 release
Motivation:
We should not use ipv4 google dns servers if the app is configured to run ipv6.
Modifications:
Use either ipv4 or ipv6 dns servers depending on the system config.
Result:
More correct behaviour
Motivation:
When the hostname portion can not be extracted we should just skip the server as otherwise we will produce and exception when trying to create the InetSocketAddress.
This was happing when trying to run the test-suite on a system and using java7:
java.lang.IllegalArgumentException: hostname can't be null
at java.net.InetSocketAddress.checkHost(InetSocketAddress.java:149)
at java.net.InetSocketAddress.<init>(InetSocketAddress.java:216)
at io.netty.util.internal.SocketUtils$10.run(SocketUtils.java:171)
at io.netty.util.internal.SocketUtils$10.run(SocketUtils.java:168)
at java.security.AccessController.doPrivileged(Native Method)
at io.netty.util.internal.SocketUtils.socketAddress(SocketUtils.java:168)
at io.netty.resolver.dns.DefaultDnsServerAddressStreamProvider.<clinit>(DefaultDnsServerAddressStreamProvider.java:74)
at io.netty.resolver.dns.DnsServerAddressesTest.testDefaultAddresses(DnsServerAddressesTest.java:39)
Modifications:
Skip if hostname can not be extracted.
Result:
No more java.lang.ExceptionInInitializerError.
Motivation:
JNDI allows to specify an port so we should respect it.
Modifications:
Use the specified port and if none is specifed use 53.
Result:
Correct handling of JNDI configured DNS.
Motivation:
DnsNameResolverTest has been observed to timeout on the CI servers. We should increase the timeout from 5 seconds to 30 seconds.
Modifications:
- Increase timeout from 5 to 30 seconds.
Result:
Less false failures due to slower CI machines.
Motivation:
The DNS resolver may use default configuration inherited from the environment. This means the ndots value may change and result in test failure if the tests don't explicitly set the assumed value.
Modifications:
- Explicitly set ndots in resolver-dns unit tests so we don't fail if the environment overrides the search domain and ndots
Result:
Unit tests are less dependent upon the enviroment they run in.
Fixes https://github.com/netty/netty/issues/6966.
Motivation:
UnixResolverDnsServerAddressStreamProvider currently throws an exception if /etc/resolver exists but it empty. This shouldn't be an exception and can be tolerated as if there is no contribution from /etc/resolver.
Modifications:
- Treat /etc/resolver as present and empty the same as not being present
Result:
UnixResolverDnsServerAddressStreamProvider initialization can tolerate empty /etc/resolver directory.
Motivation:
InetSocketAddress#getHostName() may attempt a reverse lookup which may lead to test failures because the expected address will not match.
Modifications:
- Use InetSocketAddress#getHostString() which will not attempt any lookups and instead return the original String
Result:
UnixResolverDnsServerAddressStreamProviderTest is more reliable.
Motivation:
If there are multiple DNS servers to query Java's DNS resolver will attempt to resolve A and AAAA records in sequential order and will terminate with a failure once all DNS servers have been exhausted. Netty's DNS server will share the same DnsServerAddressStream for the different record types which may send the A question to the first host and the AAAA question to the second host. Netty's DNS resolution also may not progress to the next DNS server in all situations and doesn't have a means to know when resolution has completed.
Modifications:
- DnsServerAddressStream should support new methods to allow the same stream to be used to issue multiple queries (e.g. A and AAAA) against the same host.
- DnsServerAddressStream should support a method to determine when the stream will start to repeat, and therefore a failure can be returned.
- Introduce SequentialDnsServerAddressStreamProvider for sequential use cases
Result:
Fixes https://github.com/netty/netty/issues/6926.
Motivation:
ba80fbbe05 modified the UnknownHostException to not include the search domain if the DNS query failed, but this masks what DNS query actually failed. Have the full hostname (including the search domain) provides more visibility and may help diagnose a configuration error if queries are unexpectedly failing.
Modifications:
- Remove DnsNameResolverContext#pristineHostname
Result:
UnknownHostException is more accurate and reflect what hostname actually resulted in failure.
Motivation:
DnsQueryLifecycleObserver is designed to capture the life cycle of every query. DnsNameResolverContext has a custom trace mechanism which consists of a StringBuilder and manual calls throughout the class. We can remove some special case code in DnsNameResolverContext and instead use a special implementation of DnsQueryLifecycleObserver when trace is enabled.
Modifications:
- Remove all references to the boolean trace variables in DnsNameResolverContext and DnsNameResolver
- Introduce TraceDnsQueryLifecycleObserver which will be used when trace is enabled and will log similar data as what trace currently provides
Result:
Less special case code in DnsNameResolverContext and instead delegate to TraceDnsQueryLifecycleObserver to capture trace information.
Motivation:
The DNS resolver supports search domains. However the ndots are not correctly enforced. The search domain should only be appended under the following scenario [1]:
> Resolver queries having fewer than ndots dots (default is 1) in them will be attempted using each component of the search path in turn until a match is found.
The DNS resolver current appends the search domains if ndots is 0 which should never happen (because no domain can have less than 0 dots).
[1] https://linux.die.net/man/5/resolv.conf
Modifications:
- Parse /etc/resolv.conf to get the default value for ndots on Unix platforms
- The search domain shouldn't be used if ndots is 0
- Avoid failing a promise to trigger the search domain queries in DnsNameResolverContext#resolve
Result:
More correct usage of search domains in the DNS resolver.
Fixes https://github.com/netty/netty/issues/6844.
Motivation:
It’s currently complicated to extend `DnsNameResolver` as the default
value for `searchDomain` is package private.
Modifications:
* let `DnsNameResolver` accept a null `searchDomains` and then default
to `DEFAULT_SEARCH_DOMAINS`, just like it’s being done with
`resolvedAddressTypes`.
* set default `DnsNameResolverBuilder#searchDomains` value to null to
avoid cloning internal `DnsNameResolver.DEFAULT_SEARCH_DOMAINS` in
`DnsNameResolver` constructor.
Result:
More versatile `DnsNameResolver` constructor.
No array copy when using default search domains.
Motivation:
UnixResolverDnsServerAddressStreamProvider allows the default name server address stream to be null, but there should always be a default stream to fall back to ([1] Search Strategy).
UnixResolverDnsServerAddressStreamProvider currently shuffles the names servers are multiple are present, but the defined behavior is to try them sequentially [2].
[1] Search Strategy Section - https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man5/resolver.5.html
[2] DESCRIPTION/nameserver Section - https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man5/resolver.5.html
Modifications:
- UnixResolverDnsServerAddressStreamProvider should always use the first file provided to derive the default domain server address stream. Currently if there are multiple domain names in the file identified by the first argument of the constructor then one will be selected at random.
- UnixResolverDnsServerAddressStreamProvider should return name servers sequentially.
- Reduce access level on some methods which don't have known use-cases externally.
Result:
Fixes https://github.com/netty/netty/issues/6736
Motivation:
IDN.toUnicode(...) removes trailing dots when used in Java7 while it not does on java8.
Modifications:
Check if we should test with the trailing dot removed or not.
Result:
Test pass on Java7 as well.
Motivation:
A single DNS query may follow many different paths through resolver-dns. The query may fail for various reasons related to the DNS protocol, general IO errors, it may be cancelled due to the query count being exceeded, or other reasons. A query may also result in other queries as we follow the DNS protocol (e.g. redirects, CNAME, etc...). It is currently impossible to collect information about the life cycle of an individual query though resolver-dns. This information may be valuable when considering which DNS servers are preferred over others.
Modifications:
- Introduce an interface which can provide visibility into all the potential outcomes of an individual DNS query
Result:
resolver-dns provides visibility into individual DNS queries which can be used to avoid poorly performing DNS servers.
Motivation:
1. The use of InternetProtocolFamily is not consistent:
the DnsNameResolverContext and DnsNameResolver contains switches
instead of appropriate methods usage.
2. The InternetProtocolFamily class contains redundant switches in the
constructor.
Modifications:
1. Replacing switches to the use of an appropriate methods.
2. Simplifying the InternetProtocolFamily constructor.
Result:
Code is cleaner and simpler.
Motivation:
Using reflection to obtain the default name servers may fail in Java9 and also in previous Java versions if a SecurityManager is present.
Modifications:
Try using jndi-dns to obtain default name servers and only try using reflection if this fails.
Result:
Be able to detect default name servers in all cases. Fixes [#6347].
Motivation:
DnsServerAddresses loads the default DNS servers used for DNS resolution in a static initialization block. This is subject to blocking and may cause unexpected delays. We can move this initialization to DefaultDnsServerAddressStreamProvider where it is more expected to load the JDK's default configuration.
Modifications:
- Move all the static initialization from DnsServerAddresses to DefaultDnsServerAddressStreamProvider
- Deprecate static methods in DnsServerAddresses which have moved to DefaultDnsServerAddressStreamProvider
- Remove usage of deprecated methods in DnsServerAddresses
Result:
Usage of JDK's blocking DNS resolver is not required to use resolver-dns.
Motivation:
DnsNameResolverContext completes its DNS query promise automatically
when no queries are in progress, which means there's no need to fail the
promise explicitly.
Modifications:
- Do not fail a DNS query promise explicitly but add an informational
trace
Result:
- Fixes#6600
- Unexpected exception on one question type does not fail the promise
too soon. If the other question succeeds, the query will succeed,
making the resolver more robust.
Motivation:
Recently DnsServerAddressStreamProvider was introduced to allow control for each query as to which DNS server should be used for resolution to respect the local host's default DNS server configuration. However resolver-dns also accepts a stream of DNS servers to use by default, but this stream is not host name aware. This creates an ambiguity as to which method is used to determine the DNS server to user during resolution, and in which order. We can remove this ambiguity and provide a more general API by just supporting DnsServerAddressStreamProvider.
Modifications:
- Remove the fixed DnsServerAddresses and instead only accept a DnsServerAddressStreamProvider.
- Add utility methods to help use DnsServerAddressStreamProvider for a single entry, a list of entries, and get the default for the current machine.
Result:
Fixes https://github.com/netty/netty/issues/6573.
Motivation:
We have our own ThreadLocalRandom implementation to support older JDKs . That said we should prefer the JDK provided when running on JDK >= 7
Modification:
Using ThreadLocalRandom implementation of the JDK when possible.
Result:
Make use of JDK implementations when possible.
Motivation:
54c9ecf682 introduced a unit tests which attempted to exclude addresses which resolved to loop back addresses from an assert statement. This was done with a static check for localhost but depending on machine configuration it is possible for other interfaces to be resolved.
Modifications:
- Use InetAddress#isLoopbackAddress() instead of string match on localhost
Result:
DnsNameResolverTest#testNameServerCache is more reliable.
Motivation:
The JDK uses gethostbyname for blocking hostname resoltuion. gethostbyname can be configured on Unix systems according to [1][2]. This may impact the name server that is used to resolve particular domains or just override the default fall-back resolver. DnsNameResolver currently ignores these configuration files which means the default resolution behavior is different than the JDK. This may lead to unexpected resolution failures which succeed when using the JDK's resolver.
Modifications:
- Add an interface which can override what DnsServerAddressStream to use for a given hostname
- Provide a Unix specific implementation of this interface and implement [1][2]. Some elements may be ignored sortlist, timeout, etc...
Result:
DnsNameResolver behaves more like the JDK resolver by default.
[1] https://linux.die.net/man/5/resolver
[2] https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man5/resolver.5.html
Motivation:
HostsFileParser only retains the first address for each given hostname.
This is wrong, and it’s allowed to have both an IPv4 and an IPv6.
Modifications:
* Have `HostsFileParser` now return a `HostsFileEntries` that contains IPv4 entries and IPv6 entries
* Introduce `ResolvedAddressTypes` to describe resolved address types preferences
* Add a new `ResolvedAddressTypes` parameter to `HostsFileEntriesResolver::address` to account for address types preferences
* Change `DnsNameResolver` constructor to take a `ResolvedAddressTypes`, allowing for a null value that would use default
* Change `DnsNameResolverBuilder::resolvedAddressTypes` to take a `ResolvedAddressTypes`
* Make `DnsNameResolver::resolvedAddressTypes` return a `ResolvedAddressTypes`
* Add a static `DnsNameResolverBuilder::computeResolvedAddressTypes` to ease converting from `InternetProtocolFamily`
Result:
We now support hosts files that contains IPv4 and IPv6 pairs for a same
hostname.
Motivation:
a416b79 introduced a check for null or empty host name to be compatible with the JDK resolution. However the doResolve(String, Promise) method, and if the doResolve(String, DnsRecord[], Promise, DnsCache) method was overridden the empty/null hostname would not be correctly resolved.
Modifications:
- Move the empty/null host name check into the lowest level doResolve method in DnsNameResolver
- Remove the duplicate logic in InetNameResolver.java which can be bypassed anyways
Result:
By default (unless behavior is overridden) DnsNameResolver resolves null/empty host names to local host just like the JDK.
Motivation:
We need to ensure we release the AddressedEnvelope if we fail to notify the future (as it may be notified before because of an timeout). Otherwise we may leak.
Modifications:
Call release() if we fail to notify the future.
Result:
No more memory leak on notify failure.
Motivation:
DnsNameResolver does not handle recursive DNS and so fails if you query a DNS server (for example a ROOT dns server) which provides the correct redirect for a domain.
Modification:
Add support for redirects (a.k.a. handling of AUTHORITY section').
Result:
Its now possible to use a DNS server that redirects.
Motivation:
DnsNameResolver will return the domain / host name as ascii code using punycode (https://tools.ietf.org/html/rfc3492). This is different to what the JDK does which always convert it to unicode. We should do the same by default but allow to also not do it.
Modifications:
- Add new builder method on DnsNameResolverBuilder which allow to disable / enable converting. Default is to convert just like the JDK does.
- Add unit tests for it.
Result:
DnsNameResolver and JDK impl behave the same way.
Motivation:
When an empty hostname is used in DnsNameResolver.resolve*(...) it will never notify the future / promise. The root cause is that we not correctly guard against errors of IDN.toASCII(...) which will throw an IllegalArgumentException when it can not parse its input. That said we should also handle an empty hostname the same way as the JDK does and just use "localhost" when this happens.
Modifications:
- If the try to resolve an empty hostname we use localhost
- Correctly guard against errors raised by IDN.toASCII(...) so we will always noify the future / promise
- Add unit test.
Result:
DnsNameResolver.resolve*(...) will always notify the future.
Motivation:
Currently Netty does not wrap socket connect, bind, or accept
operations in doPrivileged blocks. Nor does it wrap cases where a dns
lookup might happen.
This prevents an application utilizing the SecurityManager from
isolating SocketPermissions to Netty.
Modifications:
I have introduced a class (SocketUtils) that wraps operations
requiring SocketPermissions in doPrivileged blocks.
Result:
A user of Netty can grant SocketPermissions explicitly to the Netty
jar, without granting it to the rest of their application.
Motivation:
When we follow CNAME records we should respect resolvedAddressTypes and only query A / AAAA depending on which address types are expected.
Modifications:
Check if we should query A / AAAA when follow CNAMEs depending on resolvedAddressTypes.
Result:
Correct behaviour when follow CNAMEs.
Motivation:
We miss checking if DnsCache is null in DnsNameResolver constructor which will later then lead to a NPE. Better fail fast here.
Modifications:
Check for null and if so throw a NPE.
Result:
Fail fast.
Motivation:
In later Java8 versions our Atomic*FieldUpdater are slower then the JDK implementations so we should not use ours anymore. Even worse the JDK implementations provide for example an optimized version of addAndGet(...) using intrinsics which makes it a lot faster for this use-case.
Modifications:
- Remove methods that return our own Atomic*FieldUpdaters.
- Use the JDK implementations everywhere.
Result:
Faster code.
Motivation:
We should not use the InternalThreadLocalMap where access may be done from outside the EventLoop as this may create a lot of memory usage while not be reused anyway.
Modifications:
Not use InternalThreadLocalMap in places where the code-path will likely be executed from outside the EventLoop.
Result:
Less memory bloat.
Motivation:
Make small refactoring for recently merged PR #5867 to make the code more flexible and expose aggressive round robin as a NameResolver too with proper code reuse.
Modifications:
Round robin is a method of hostname resolving - so Round robin related code fully moved to RoundRobinInetAddressResolver implements NameResolver<InetAddress>, RoundRobinInetSocketAddressResolver is deleted as a separate class, instance with the same functionality could be created by calling #asAddressResolver.
Result:
New forced Round Robin code exposed not only as an AddressResolver but as a NameResolver too, more proper code and semantic reusing of InetNameResolver and InetSocketAddressResolver classes.
Motivation:
Suppose the domain `foo.example.com` resolves to the following ip
addresses `10.0.0.1`, `10.0.0.2`, `10.0.0.3`. Round robin DNS works by
having each client probabilistically getting a different ordering of
the set of target IP’s, so connections from different clients (across
the world) would be split up across each of the addresses. Example: In
a `ChannelPool` to manage connections to `foo.example.com`, it may be
desirable for high QPS applications to spread the requests across all
available network addresses. Currently, Netty’s resolver would return
only the first address (`10.0.0.1`) to use. Let say we are making
dozens of connections. The name would be resolved to a single IP and
all of the connections would be made to `10.0.0.1`. The other two
addresses would not see any connections. (they may see it later if new
connections are made and `10.0.0.2` is the first in the list at that
time of a subsequent resolution). In these changes, I add support to
select a random one of the resolved addresses to use on each resolve
call, all while leveraging the existing caching and inflight request
detection. This way in my example, the connections would be make to
random selections of the resolved IP addresses.
Modifications:
I added another method `newAddressResolver` to
`DnsAddressResolverGroup` which can be overriden much like
`newNameResolver`. The current functionality which creates
`InetSocketAddressResolver` is still used. I added
`RoundRobinDnsAddressResolverGroup` which extends
DnsAddressResolverGroup and overrides the `newAddressResolver` method
to return a subclass of the `InetSocketAddressResolver`. This subclass
is called `RoundRobinInetSocketAddressResolver` and it contains logic
that takes a `resolve` request, does a `resolveAll` under the hood, and
returns a single element at random from the result of the `resolveAll`.
Result:
The existing functionality of `DnsAddressResolverGroup` is left
unchanged. All new functionality is in the
`RoundRobinInetSocketAddressResolver` which users will now have the
option to use.
Motivation:
RFC7871 defines an extension which allows to request responses for a given subset.
Modifications:
- Add DnsOptPseudoRrRecord which can act as base class for extensions based on EDNS(0) as defined in RFC6891
- Add DnsOptEcsRecord to support the Client Subnet in DNS Queries extension
- Add tests
Result:
Client Subnet in DNS Queries extension is now supported.
Motivation:
According to the Oracle documentation:
> java.net.preferIPv4Stack (default: false)
>
> If IPv6 is available on the operating system, the underlying native
> socket will be an IPv6 socket. This allows Java applications to connect
> to, and accept connections from, both IPv4 and IPv6 hosts.
>
> If an application has a preference to only use IPv4 sockets, then this
> property can be set to true. The implication is that the application
> will not be able to communicate with IPv6 hosts.
which means, if DnsNameResolver returns an IPv6 address, a user (or
Netty) will not be able to connect to it.
Modifications:
- Move the code that retrieves java.net.prefer* properties from
DnsNameResolver to NetUtil
- Add NetUtil.isIpV6AddressesPreferred()
- Revise the API documentation of NetUtil.isIpV*Preferred()
- Set the default resolveAddressTypes to IPv4 only when
NetUtil.isIpv4StackPreferred() returns true
Result:
- Fixes#5657
Motivation:
AddressResolverGroup adds a listener to the termination future of an
EventExecutor when a new AddressResolver is created. The listener calls
AddressResolver.close() when the EventExecutor is terminated to give the
AddressResolver a chance to release its resources.
When using DnsAddressResolverGroup, the AddressResolver.close() will
eventually trigger DnsNameResolver.close(), which closes its underlying
DatagramChannel.
DatagramChannel.close() (or any Channel.close()) will travel through
pipeline and trigger EventExecutor.execute() because
DnsNameResolver.close() has been invoked from a non-I/O thread.
(NB: A terminationFuture is always notified from the GlobalEventExecutor
thread.)
However, because we are doing this in the listener of the termination
future of the terminated EventLoop we are trying to execute a task upon,
the attempt to close the channel fails due to RejectedExecutionException.
Modifications:
- Do not call Channel.close() in DnsNameResolver.close() if the Channel
has been closed by EventLoop already
Result:
No more RejectedExecutionException when shutting down an event loop.
Motivation:
When a hostname cannot be resolved, the message in the UnknownHostException mentions the hostname with the last attempted search domain appended, which is kind of confusing. I would prefer to see the original hostname supplied to the method in the exception.
Modifications:
Store the pristine hostname in the resolver context and use it to create the exception message instead of the hostname with search domain.
Add unit test to check that the exception does not mention the search domain.
Result:
The exception mentions the unmodified hostname in the message.
Motivation:
The ndots = 0 is a valid value for ndots, it means that when using a non dotted name, the resolution should first try using a search and if it fails then use subdomains. Currently it is not allowed. Docker compose uses this when wiring up containers as names have usually no dots inside.
Modification:
Modify DnsNameResolver to accept ndots = 0 and handle the case in the resolution procedure. In this case a direct search is done and then a fallback on the search path is performed.
Result:
The ndots = 0 case is implemented.
Motivation:
When resolving localhost on Windows where the hosts file does not contain a localhost entry by default, the resulting InetAddress object returned by the resolver does not have the hostname set so that getHostName returns the ip address 127.0.0.1. This behaviour is inconsistent with Windows where the hosts file does contain a localhost entry and with Linux in any case. It breaks at least some unit tests.
Modifications:
Create the LOCALHOST4 and LOCALHOST6 objects with hostname localhost in addition to the address.
Add unit test domain localhost to DnsNameResolverTest to check the resolution of localhost with ipv4 at least.
Result:
The resolver returns a InetAddress object for localhost with the hostname localhost in all cases.
Motivation:
The current DnsNameResolver does not support search domains resolution. Search domains resolution is supported out of the box by the java.net resolver, making the DnsNameResolver not able to be a drop in replacement for io.netty.resolver.DefaultNameResolver.
Modifications:
The DnsNameResolverContext resolution has been modified to resolve a list of search path first when it is configured so. The resolve method now uses the following algorithm:
if (hostname is absolute (start with dot) || no search domains) {
searchAsIs
} else {
if (numDots(name) >= ndots) {
searchAsIs
}
if (searchAsIs wasn't performed or failed) {
searchWithSearchDomainsSequenciallyUntilOneSucceeds
}
}
The DnsNameResolverBuilder provides configuration for the search domains and the ndots value. The default search domains value is configured with the OS search domains using the same native configuration the java.net resolver uses.
Result:
The DnsNameResolver performs search domains resolution when they are present.
Modifications:
The DnsNameResolver Bootstrap does not bind anymore, instead it registers and use the channel directly. The localAddress has also been removed from the DnsAddressResolverGroup, DnsNameResolver and DnsNameResolverBuilder as it is not necessary anymore and the API is marked as @UnstableApi.
Result:
Dns resolution does not require anymore to bind locally.
Motivation:
If DnsNameResolver works with NoopDnsCache, IndexOutOfBoundsException will
be thrown.
Modifications:
Test if the result of DnsNameResolver.get(hostname) is empty before
accessing it's elements.
Motivation:
On Windows localhost is not in hosts file and the DNS server does not resolve this address either, i.e it is handled by the Windows API. So using a Bootstrap (among others) with the resolver based on DnsNameResolver will not resolve localhost.
Modifications:
Workaround behavior of Windows
Result:
Correctly resolve localhost on Windows when using DnsNameResolver
Motivation:
The current DnsNameResolver fails to resolve an A+CNAME answer. For example:
dig moose.rmq.cloudamqp.com
...
;; ANSWER SECTION:
moose.rmq.cloudamqp.com. 1800 IN CNAME ec2-54-152-221-139.compute-1.amazonaws.com.
ec2-54-152-221-139.compute-1.amazonaws.com. 583612 IN A 54.152.221.139
...
The resolver constructs a map of cnames but forgets the trailing "." in the values which lead to not resolve the A record.
Modifications:
Reuse the code of DefaltDnsRecordDecoder which correctly handles the trailing dot.
Result:
Correctly resolve.
Motivation:
JCTools supports both non-unsafe, unsafe versions of queues and JDK6 which allows us to shade the library in netty-common allowing it to stay "zero dependency".
Modifications:
- Remove copy paste JCTools code and shade the library (dependencies that are shaded should be removed from the <dependencies> section of the generated POM).
- Remove usage of OneTimeTask and remove it all together.
Result:
Less code to maintain and easier to update JCTools and less GC pressure as the queue implementation nt creates so much garbage
Motivation:
We use a default of 3 for maxQueriesPerResolve when using the DnsNameResolverBuilder, which is too low if you want to resolve a hostname that uses a lot of CNAME records.
Modifications:
- Use higher default (16)
- Make exception message more clear why it failed.
Result:
Be able to resolve more domains by default and be able to better trouble shoot why a resolver failed.
Motivation:
DnsAddressResolverGroup allows to override the newResolver(...) method to change the settings used by the user. We should better let the user override another method and always apply the InflightNameResolver.
Modifications:
- Mark newResolver(...) method as deprecated, we will make it private soon.
- Add newNameResolver(...) method that user can override.
Result:
Easier to extend DnsAddressResolverGroup
Related issue: #5179
Motivation:
When you attempt to make a lot of connection attempts to the same target
host at the same time and our DNS resolver does not have a record for it
in the cache, the DNS resolver will send as many DNS queries as the
number of connection attempts.
As a result, DNS server will reject or drop the requests, making the
name resolution attempt fail.
Modifications:
- Add InflightNameResolver that keeps the list of name resolution
queries and subscribes to the future of the matching query instead of
sending a duplicate query.
Result:
- The AddressResolvers created by DnsAddressResolverGroup do not send
duplicate DNS queries anymore
Motivation:
Some codecs should be considered unstable as these are relative new. For this purpose we should introduce an annotation which these codecs should us to be marked as unstable in terms of API.
Modifications:
- Add UnstableApi annotation and use it on codecs that are not stable
- Move http2.hpack to http2.internal.hpack as it is internal.
Result:
Better document unstable APIs.
Motivation:
There were some warning in the resolver-dns code base.
Modifications:
- Fix javadocs
- Use the base class to call static method.
Result:
Cleaner code.
Related: #4771
Motivation:
A malicious or misconfigured DNS server can send the CNAME records that
resolve into each other, causing an unexpected infinite loop in
DnsNameResolverContext.onResponseCNAME().
Modifications:
- Remove the dereferenced CNAME from the alias map so that infinite loop
is impossible.
- Fix inspection warnings and typos in DnsNameResolverTest
Result:
Fixes#4771
Motivation:
Current DnsNameResolver api don't allow to define additional records in DNS query.
It can be useful in many cases. For example when we want to query dns server with
real client address (EDNS-CLIENT-SUBNET extension:
http://tools.ietf.org/html/draft-vandergaast-edns-client-subnet-02 )
Modifications:
This change add new query methods with list of additional DnsRecord-s for query.
Result:
It is possible to create dns query with EDNS-CLIENT-SUBNET extension for example.
Motivation:
See #3411. A reusable ArrayList in InternalThreadLocalMap can avoid allocations in the following pattern:
```
List<...> list = new ArrayList<...>();
add something to list but never use InternalThreadLocalMap
return list.toArray(new ...[list.size()]);
```
Modifications:
Add a reusable ArrayList to InternalThreadLocalMap and update codes to use it.
Result:
Reuse a thread local ArrayList to avoid allocations.
Motivation:
DnsNameResolverBuilder#nameServerAddresses isn’t initialized with a
default value. In most cases, user will want
DefaultDnsServerAddresses#defaultAddresses.
Modifications:
Initialize DnsNameResolverBuilder#nameServerAddresses with
DefaultDnsServerAddresses#defaultAddresses
Result:
DnsNameResolverBuilder more convenient usage.
Motivation:
As "getHostName" may do a reverse name lookup and return a host name based on the system configured name lookup service, testResolveIp may fail in some special environment. See #4720
Modifications:
Use getHostAddress instead of getHostName
Result:
testResolveIp works in all environments
Motivation:
Caching is currently nested in DnsResolver.
It should also be possible to extend DnsResolver to ba able to pass a different cache on each resolution attemp.
Modifications:
* Introduce DnsCache, NoopDnsCache and DefaultDnsCache. The latter contains all the current caching logic that was extracted.
* Introduce protected versions of doResolve and doResolveAll that can be used as extension points to build resolvers that bypass the main cache and use a different one on each resolution.
Result:
Isolated caching logic. Better extensibility.
Motivation:
There are some wrong links and tags in javadoc.
Modifications:
Fix the wrong links and tags in javadoc.
Result:
These links will work correctly in javadoc.
Motivation:
On contrary to `DefaultNameResolver`, `DnsNameResolver` doesn't currently honor hosts file.
Modifications:
* Introduce `HostsFileParser` that parses `/etc/hosts` or `C:\Windows\system32\drivers\etc\hosts` depending on the platform
* Introduce `HostsFileEntriesResolver` that uses the former to resolve host names
* Make `DnsNameResolver` check his `HostsFileEntriesResolver` prior to trying to resolve names against the DNS server
* Introduce `DnsNameResolverBuilder` so we now have a builder for `DnsNameResolver`s
* Additionally introduce a `CompositeNameResolver` that takes several `NameResolver`s and tries to resolve names by delegating sequentially
* Change `DnsNameResolver.asAddressResolver` to return a composite and honor hosts file
Result:
Hosts file support when using `DnsNameResolver`.
Consistent behavior with JDK implementation.
Motivation:
As discussed in #4529, NameResolver design shouldn't be resolving SocketAddresses (or String name + port) and return InetSocketAddresses. It should resolve String names and return InetAddresses.
This SocketAddress to InetSocketAddresses resolution is actually a different concern, used by Bootstrap.
Modifications:
Extract SocketAddress to InetSocketAddresses resolution concern to a new class hierarchy named AddressResolver.
These AddressResolvers delegate to NameResolvers.
Result:
Better separation of concerns.
Note that new AddressResolvers generate a bit more allocations because of the intermediate Promise and List<InetAddress>.
Motivation:
There's no way to override the default settings of the DnsNameResolvers
created by DnsNameResolverGroup because DnsNameResolverGroup is final.
Modifications:
- Make DnsNameResolverGroup non-final
- Add a new overridable protected method 'newResolver()' so that a user
can override it to create an alternative DnsNameResolver instance or
set the non-default properties
Result:
A user can configure the DnsNameResolver.
Motivation:
Each server should be checked for every record type. Currently, if there
are only two configured servers and the first is down, it is impossible
to query for IPv4 addresses because the second server is only ever
queried for type AAAA.
Modifications:
Do not cycle DNS servers while cycling DNS record types (A and AAAA)
Result:
Name resolution is less fragile when the number of available DNS servers
is 2.
Related: #3972
Motivation:
DnsNameResolver limits the number of concurrent in-progress DNS queries
to 65536 regardless the number of DNS servers it communicates with. When
the number of available DNS servers are more than just one, we end up
using much less (65536 / numDnsServers) query IDs per DNS server, which
is non-optimal.
Modifications:
- Replace the query ID and context management with
DnsQueryContextManager
- Eash DNS server gets its own query ID space
Result:
Much bigger query ID space, and thus there's less chance of getting the
'query ID space exhaustion' error
Motivation:
When DnsNameResolverContext succeeds to get the address(es), it cancels
the promise of other queries in progress.
Unlike expectation, DnsNameResolverContext.query() attempts to retry
even when the query has failed due to cancellation.
As a result, the resolver sends unnecessary extra queries to a DNS
server and then tries to mark the promised that's been fulfilled
already, leading to unnecessarily verbose 'failed to notify success to a
promise' messages.
Modifications:
Do not perform an extra query when the previous query has failed due to
cancellation
Result:
DnsNameResolver does not send unnecessary extra queries and thus does
not log the 'failed to notify success to a promise' message.
Motivation:
As relaying on external DNS Server can result to test-failures we should better use a mock DNS Server for the dns tests.
Modifications:
- Refactor the DnsNameResolverTest to use a mock DNS Server which is using apacheds.
- Allow to disable adding an opt resources as some servers not support it.
Result:
More stable testsuite.
Motivation:
Some DNS servers in DnsNameResolverTest are outdated and some of them
returns NoError for non-existent domains.
Modifications:
- Update the DNS server list from http://meo.ws/dnsrec.php again
- Update the web-scraper script
Result:
DnsNameResolverTest.testNegativeTtl() should not fail anymore.
