Race condition in AsyncDnsResolver

[andreezy777]

Hello guys,

It looks like we have a race condition caused by this var requestId

https://github.com/akka/akka/blob/5e9e11b5f6df03ed89c5ca37bdbb295ff5c19ec4/akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala#L78

We have 3 grpc clients that are trying to initialize, so they are sending address lookup requests to the dns service.
Here are the logs that we got from AsyncDnsResolver :

{"timestamp":"2023-04-03T11:49:50.410Z","level":"DEBUG","logger":"akka.io.dns.internal.AsyncDnsResolver","message":"Attempting to resolve X.namespace1.svc.cluster.local.svc.cluster.local with Actor[akka://testSystem/system/IO-DNS/async-dns/$a/$a#658321458]"}
{"timestamp":"2023-04-03T11:49:50.411Z","level":"DEBUG","logger":"akka.io.dns.internal.AsyncDnsResolver","message":"Attempting to resolve Y.namespace2.svc.cluster.local with Actor[akka://testSystem/system/IO-DNS/async-dns/$a/$a#658321458]"}
{"timestamp":"2023-04-03T11:49:50.420Z","level":"DEBUG","logger":"akka.io.dns.internal.AsyncDnsResolver","message":"Ip(true,true) resolved Resolved(X.namespace1.svc.cluster.local.svc.cluster.local,Vector(ARecord(Y.namespace2.svc.cluster.local,Ttl(26 seconds),/100.68.87.33)),Vector())"}

....

{"timestamp":"2023-04-03T11:49:55.428Z","level":"INFO","logger":"akka.io.dns.internal.AsyncDnsResolver","message":"Resolve of Y.namespace2.svc.cluster.local timed out after 5.000 s. Trying next name server"}

As a possible solution, I can suggest using AtomicInteger to avoid race condition.

[leviramsey]

That var is private to the actor and not referenced in any callbacks, so not subject to race conditions.

About how many "Attempting to resolve" logs are there?

[andreezy777]

But this var is incremented by the nextId() method.
And this nextId() method is called by the resolveWithResolvers function, which returns Future.
This means that subsequent messages can be processed by the receive function while the future function is being performed on the previous message.
Am I wrong?

About how many "Attempting to resolve" logs are there?

We have 4 search domains.
So for host Y.namespace2 - there was 4 logs with "Attempting"
And for X.namespace1 - only 2, because it was "resolved" on second attempt.(resolved as another host as u can see by logs)

[octonato]

I think the issue happens here:

https://github.com/akka/akka/blob/5e9e11b5f6df03ed89c5ca37bdbb295ff5c19ec4/akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala#L184

nextId can be running inside a Future when we call resolve.flatMap(.. resolveFirst).

[leviramsey]

@octonato and also in https://github.com/akka/akka/blob/5e9e11b5f6df03ed89c5ca37bdbb295ff5c19ec4/akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala#L130

recoverWith trampolines back into resolveWithResolvers

[octonato]

yep, I missed the recoverWith.

My eyes only see map and flatMap. 😄

[andreezy777]

@leviramsey May I ask if there will be a backport of these changes to older versions of the akka (2.7, 2.6 at least)?

[He-Pin]

I think there is a bug in DnsClient too. What if a large number of queries is issued and then the nextId will round over from Short.MinValue to Short.MaxValue.

If we have two queries with different targets but with the same nextId, then there will be a mess up.

1. when handling a query, if there is an in-flight query exists in inflightRequests, then stash the query or just drop it.
2. when handling a query, schedule a query timeout for removing the record from inflightRequests if not receive result for a longtime and fail the query.

As I never used this module but I still think there is an issue exists with the current code.

[johanandren]

@andreezy777 we don't expect to backport this fix unless we find some strong motivation for it, 2.8.x is a drop in replacement for 2.7 so that should be an easy upgrade once the fix is released.

[andreezy777]

@johanandren

One more question about 2.6 version.

Did I understand correctly that this bug is not critical?
So it won't be backported to 2.6?

[johanandren]

Correct, we don't consider this a critical bug and are not currently planning to backport the fix.

[andreezy777]

@johanandren

I want to describe our situation so that you can understand why we think this is a critical bug.

In our case, we use akka-grpc and in some services we have 1+ grpc clients, so we sometimes face this problem, and it is quite critical because requests are lost and quick solutions are to restart service so to make dns resolver work properly or hope that there are no requests and after idle-timeout the correct dns resolution will occur for next requests.

Accordingly, in addition to updating the akka version, we need to update akka-grpc and all dependencies, test, and it may take a long time before we update.

A minor version update to 2.6 would help a lot, as it would be possible to stabilize grpc on production relatively quickly.

Therefore, if you would be so kind, I would like to ask you to reconsider the decision on the criticality of this functionality and backport it to 2.6.
Thanks in advance.