mdedetrich commented on code in PR #385: URL: https://github.com/apache/incubator-pekko/pull/385#discussion_r1227713130
########## actor/src/main/scala/org/apache/pekko/io/dns/IdGenerator.scala: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * license agreements; and to You under the Apache License, version 2.0: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * This file is part of the Apache Pekko project, derived from Akka. + */ + +package org.apache.pekko.io.dns + +import org.apache.pekko.annotation.InternalApi + +import java.security.SecureRandom +import java.util.concurrent.ThreadLocalRandom +import java.util.concurrent.atomic.AtomicInteger +import scala.annotation.tailrec + +/** + * INTERNAL API + * + * These are called by an actor, however they are called inside composed futures so need to be + * nextId needs to be thread safe. + */ +@InternalApi +private[pekko] trait IdGenerator { + def nextId(): Short +} + +/** + * INTERNAL API + */ +@InternalApi +private[pekko] object IdGenerator { + sealed trait Policy + + object Policy { + case object Sequence extends Policy + case object ThreadLocalRandom extends Policy + case object SecureRandom extends Policy + val Default: Policy = ThreadLocalRandom + + def apply(name: String): Option[Policy] = name.toLowerCase match { + case "sequence" => Some(Sequence) + case "thread-local-random" => Some(ThreadLocalRandom) + case "secure-random" => Some(SecureRandom) + case _ => Some(ThreadLocalRandom) + } + } + + def apply(policy: Policy): IdGenerator = policy match { + case Policy.Sequence => sequence() + case Policy.ThreadLocalRandom => random(ThreadLocalRandom.current()) + case Policy.SecureRandom => random(new SecureRandom()) + } + + /** + * @return a random sequence of ids for production + */ + def random(rand: java.util.Random): IdGenerator = new IdGenerator { + override def nextId(): Short = rand.nextInt(Short.MaxValue).toShort + } + + /** + * @return a predictable sequence of ids for tests + */ + def sequence(): IdGenerator = new IdGenerator { Review Comment: My concern here is that while this is definitely useful for tests I don't like the code being there for it to be accidentally used in production considering the security ramifications. Its actually possible to modify the tests so it can handle random ID's and I would argue that its actually beneficial rather than providing a sequential generator since you are testing intended real world behaviour rather than against hardcoded id's. Put differently, you are actually testing the "get id from initial resolve request and then use that retrieved id further requests" which is what happens in reality. Thankfully the hard work has already been done here, see https://github.com/apache/incubator-pekko/pull/371/files#diff-17ee4dc4858fd137eeb621bf70244df69a1c97eeb5f9d178699c74fbc500dd6c . You can copy that and it should just work. @IainHull @pjfanning @He-Pin wdyt? -- 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]
