fix: Refactor concurrency in AsyncDnsResolver #31906

leviramsey · 2023-04-12T16:33:44Z

References #31905

Future callbacks in AsyncDnsResolver will, in situations where multiple resolution attempts for a request are being made (e.g. failing over to a different DNS server or if there are multiple search domains), close over actor state which is not otherwise synchronized, thus breaking the actor model's single threaded illusion.

This change moves such resolution attempts to explicit actor messages.

leviramsey · 2023-04-12T16:37:48Z

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala


  import AsyncDnsResolver._

-  implicit val ec: ExecutionContextExecutor = context.dispatcher


I do kind of feel that having an implicit execution context in an actor is a bad sign, by decreasing the cognitive cost of adding a future callback within the actor.

leviramsey · 2023-04-12T16:42:47Z

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

-              log.debug(s"{} resolved {}", mode, resolved)
-              resolved
+          val replyTo = sender()
+          resolveWithResolvers(name, mode, resolvers).onComplete {


Broad idea is to limit the future callbacks inside the actor. Combining the cache updates with piping preserves the sequencing in .map { x => sideEffect(); x }.pipeTo(sender()) and allows use of parasitic for performance without making that the default context.

maybe pipeTo is better .

Yes, +100 for parasitic

leviramsey · 2023-04-12T16:43:54Z

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

+          ipv6Recs.map { v6 =>
+            DnsProtocol.Resolved(name, v4.rrs ++ v6.rrs, v4.additionalRecs ++ v6.additionalRecs)
+          }(ExecutionContexts.parasitic)
+        }(ExecutionContexts.parasitic))


Again, personal rule to never implicit val ec = ExecutionContexts.parasitic...

leviramsey · 2023-04-12T18:04:43Z

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

+                  val minTtl = (positiveCachePolicy +: resolved.records.map(_.ttl)).min
+                  cache.put((name, mode), resolved, minTtl) // thread-safe structure, safe in callback
+                } else if (negativeCachePolicy != Never) {
+                  cache.put((name, mode), resolved, negativeCachePolicy) // thread-safe structure, safe in callback


If it was going to just be a Map, definitely. In this case, since we're going through the expense of using a thread-safe structure, may as well exercise that :)

This does have the benefit of quickly allowing a successful resolution to immediately update the cache for requests which might be in the mailbox now (without having to do a custom mailbox). Since the only cache.get is right when we receive a DnsProtocol.Resolve, it's not much of a win (it might be a slight win under heavy load to check the cache when making follow-up queries).

patriknw · 2023-04-13T06:43:49Z

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala


  import AsyncDnsResolver._

-  implicit val ec: ExecutionContextExecutor = context.dispatcher


akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

patriknw · 2023-04-13T07:30:12Z

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

-  }
-
-  private def resolveWithSearch(
+  private def resolvePromise(


Do I understand it correctly that the only reason for going back to the actor via Internal.Resolve is that the nextId() is needed here, or is there something else that requires this to be running in the actor?

If that is the case, would it be better to change the requestId counter to an a thread safe counter (AtomicInteger) and stay in the Future land in the companion object methods?

octonato

I left a few comments, but need to think more about it.

Maybe we need to explore other strategies.

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

This reverts commit d15509b. Alternate approach

…'t need to immediately spawn

leviramsey · 2023-04-14T17:10:16Z

Trying a different approach:

move requestId responsibility to a dedicated actor
the progression through search names and different resolvers is now the responsibility of a per-request actor. This does allow for some optimizations (e.g. only building a list of names with search domains appended once), at the expense of not guaranteeing that the cache update (if any) happens before the requestor receives the response

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

patriknw · 2023-04-17T07:20:05Z

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

+    }
+  }
+
+  private class DnsResolutionActor(


Looks good to me to have this per request actor.

I'm not sure I was consciously thinking about this, but I like the separation of AsyncDnsResolver is primarily managing the cache, the per-request actor is managing the process of preparing requests to resolvers, and the injector is keeping the requestIds sane.

The per-request actor is as pasta-like as the future code was (maybe the invariants are clearer?), but is a bit more encapsulated.

leviramsey · 2023-04-17T17:22:25Z

Test failure looks to be unrelated (actorsystem logging): https://github.com/akka/akka/actions/runs/4723246264/jobs/8378944101#step:5:1074

retriggering

He-Pin · 2023-04-17T18:49:58Z

As only one place to generate the next id, if only from a performance perspective, i don't think this would performant better than a simple atomic integer with updateAndGet.

But still ♡(´∀｀)人(´∀｀)♡

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

johanandren · 2023-04-18T08:31:48Z

close-reopen to retrigger ci

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

johanandren

Much easier to follow now, good.

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala

johanandren

LGTM

patriknw

LGTM

* Alternative approach: per-request actor * Restore synchronous 'resolution' for situations where the actor doesn't need to immediately spawn (cherry picked from commit ef33e71)

fix: Refactor future callbacks in AsyncDnsResolver

d15509b

leviramsey commented Apr 12, 2023

View reviewed changes

patriknw reviewed Apr 13, 2023

View reviewed changes

octonato reviewed Apr 13, 2023

View reviewed changes

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala Outdated Show resolved Hide resolved

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala Outdated Show resolved Hide resolved

leviramsey added 3 commits April 13, 2023 11:00

Revert "fix: Refactor future callbacks in AsyncDnsResolver"

9a40ec7

This reverts commit d15509b. Alternate approach

Alternative approach: per-request actor

0c23d0c

Restore synchronous 'resolution' for situations where the actor doesn…

7746f54

…'t need to immediately spawn

leviramsey added 3 commits April 14, 2023 13:17

style

cb3e46e

searchName vs. name...

17ec0a9

scala 3

714029c

patriknw reviewed Apr 17, 2023

View reviewed changes

feedback, simplify per-request actor a little bit

146e872

patriknw reviewed Apr 18, 2023

View reviewed changes

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala Show resolved Hide resolved

johanandren closed this Apr 18, 2023

johanandren reopened this Apr 18, 2023

johanandren reviewed Apr 18, 2023

View reviewed changes

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala Outdated Show resolved Hide resolved

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala Outdated Show resolved Hide resolved

leviramsey added 2 commits April 18, 2023 10:35

drop unused message

a24bb14

simplify the state machine... look ma, no stash!

7999012

leviramsey changed the title ~~fix: Refactor future callbacks in AsyncDnsResolver~~ fix: Refactor concurrency in AsyncDnsResolver Apr 18, 2023

johanandren reviewed Apr 19, 2023

View reviewed changes

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala Outdated Show resolved Hide resolved

akka-actor/src/main/scala/akka/io/dns/internal/AsyncDnsResolver.scala Outdated Show resolved Hide resolved

(simplification of)^n

23f7212

leviramsey mentioned this pull request Apr 19, 2023

Use parasitic context by default in Classic pipe pattern #31917

Open

johanandren approved these changes Apr 20, 2023

View reviewed changes

johanandren added the 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding label Apr 20, 2023

patriknw approved these changes Apr 21, 2023

View reviewed changes

patriknw merged commit ef33e71 into akka:main Apr 21, 2023

leviramsey deleted the async-dns-concurrency branch April 21, 2023 14:23

johanandren mentioned this pull request Apr 24, 2023

=act Fix dns query overriding in dnsClient. #31912

Closed

nodefactory-bk mentioned this pull request Sep 19, 2023

AsyncDns hang/deadlock causing permanent timeouts resolving affected names. #32109

Closed


		import AsyncDnsResolver._

		implicit val ec: ExecutionContextExecutor = context.dispatcher

fix: Refactor concurrency in AsyncDnsResolver #31906

fix: Refactor concurrency in AsyncDnsResolver #31906

Uh oh!

Conversation

leviramsey commented Apr 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leviramsey Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octonato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leviramsey commented Apr 14, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leviramsey commented Apr 17, 2023

Uh oh!

He-Pin commented Apr 17, 2023

Uh oh!

Uh oh!

johanandren commented Apr 18, 2023

Uh oh!

Uh oh!

Uh oh!

johanandren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johanandren left a comment

Choose a reason for hiding this comment

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leviramsey Apr 12, 2023 •

edited

Loading