mdedetrich commented on PR #371: URL: https://github.com/apache/incubator-pekko/pull/371#issuecomment-1580973268
Regarding the hash collision problem, this is either going to be non trivial to implement or use a lot of memory and not be correct. A trivial/simple way to implement this is to just have a growing bounded queue which maintains all of the ids that have been generated and to check against if a collision has occurred, dropping the latest elements of the queue if it hits the bound. The problem here is that its not a proper solution (its still theoretically possible to create a collision) and it uses a lot of memory. The proper solution which other DNS resolvers do (such as systemd-resolverd) is that whenever a new id is created its checked against the current in flight ids and if there is a duplicate then we re-create the id. The reason why we track against current in transaction ids is that hash collisions are fine if there is no context of the id of the colliding hashes, its only a problem if there are currently existing transactions that contain the newly created colliding id. Thing is, the proper solution is quite complicated and this is due to having to share state, and since we are dealing with actors you are not meant to share any mutable state, instead you are meant to be sending messages (thats the whole point of actors). The state in question that we need to access is https://github.com/apache/incubator-pekko/blob/4ac0f00a477873965ee7d52e16faefb1de91fe3a/actor/src/main/scala/org/apache/pekko/io/dns/internal/DnsClient.scala#L58 (specifically we only care about the keys of the `Map`). Initially I thought it would have been possible to just have a `currentIds` collection inside of `AsyncDnsResolver`, adding id's is easy since `nextId()` is also defined inside of `AsyncDnsResolver`, the problematic part is removing the id's. If you check occurrences of `inflightRequests -= <ID>` you can see there are 3 cases where id's are removed (i.e. dropped requests, failed UDP packets etc etc) and only in one of those cases (specifically `DropRequest`) does `AsyncDnsResolver` have context of this. In conclusion, the proper solution for this would be to add message types to `DnsQuestion`, i.e. `AskIpsInFlight` and `AnswerIpsInFlight`, implement them and then do a scatter + gather implementation in `AsyncDnsResolver` to see if a specific id is inside all of the [resolvers](https://github.com/apache/incubator-pekko/blob/8c346e215e499ed5abcdcf5f2857c3fa3b979dc4/actor/src/main/scala/org/apache/pekko/io/dns/internal/AsyncDnsResolver.scala#L110). Since this is performance sensitive code we shouldn't be using `?`, which further complicates the problem. I mean a very hacky way to solve this would be to access the state in `DnsClient` actor directly, but that would open up a huge can of works. What does everyone think? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
