Make UUIDGen more performant

[diogocanut]

On this PR I'm trying to address #2882 .

[armanbilge]

Nice start!!

[armanbilge]

I think we're almost there ... this PR turned out to be incredibly complicated to figure out how to organize everything. Suffice to say, very very nicely done with this, thanks for persevering 💯

[armanbilge]

Otherwise it will create a new one each time :)

Suggested change

	implicit def secureRandom: SecureRandom[IO] =
	lazy val secureRandom: SecureRandom[IO] =

---

Call-out for other reviewers: are we okay about unsafely creating a global secure random like this? I think having this would greatly enhance UX, but it creates a muddy precedent to use a pattern that has gone wrong sometimes e.g. FS2 Network ...

[diogocanut]

Hmm, how to add the implicit to scope having it as lazy val?

[warn] /Users/diogocanut/Projects/cats-effect/core/shared/src/main/scala/cats/effect/IO.scala:1447:37: method fromSync in trait UUIDGenCompanionPlatformLowPriority is deprecated (since 3.6.0): Put an implicit `SecureRandom.javaSecuritySecureRandom` into scope to get a more efficient `UUIDGen`, or directly call `UUIDGen.fromSecureRandom`
[warn]   def randomUUID: IO[UUID] = UUIDGen[IO].randomUUID

[armanbilge]

Oh sorry, I meant to write implicit lazy val :)

[durban]

@armanbilge What was the problem in FS2 Network?

[armanbilge]

Well, it's a rather involved discussion :) so the problem is that FS2 promises an implicit Network[IO], but this is technically encapsulating some global resources (namely, I/O threads). This was working well enough, until we started the polling system work. It created a significant challenge, since now we have to thread the IOApps polling system into this globally promised Network[IO].

If instead FS2 had been offering a Resource[IO, Network[IO]] that you had to construct explicitly, it would have been a lot easier to work with.

The fundamental question here is, when is it okay to "cheat" and unsafely allocate global shared state or resources, instead of forcing it to be explicitly allocated in IO or Resource by the user. It's some kind of UX trade-off.

See a similar place where we faced this in:

• Make Console#readLine cancelable #3465

[durban]

Thanks, I think I understand. Btw, I'm not convinced a Random[IO] must be implicitly available: there isn't "one obvious preferable" Random[IO]. However, for SecureRandom, there could be: in fact, this PR tries to do that... so I think I'm okay with it in this specific case. (But it's possible i just don't appreciate the possible future pain this could cause :-)

[armanbilge]

One last minor thing, I think!

[durban]

The other overload above (line 114) refers to this method. However, the behavior of the 2 are now quite different. I think the other overload could also benefit from the new improvements, although the n: Int does not make a lot of sense for the best case (nonblocking) PRNG.

[diogocanut]

Yes, for the best case n: Int. It will only sense in the worst case, when we fall back to a pool.

  private def pool[F[_]: Sync](n: Int): SecureRandom[F] = {
    val index = new AtomicInteger(0)
    val array = Array.fill(n)(new SRandom(new SecureRandom.JavaSecureRandom))

    def selectRandom: F[SRandom] = Sync[F].delay {
      val currentIndex = index.getAndUpdate(i => if (i < (n - 1)) i + 1 else 0)
      array(currentIndex)
    }

    new ScalaRandom[F](selectRandom) with SecureRandom[F] {}
  }

[durban]

I wasn't very clear: I'm thinking about the fact that the method on line 116 is creating an rng which is "sharded", but calls (potentially) blocking code in delay. The latter is solved by this PR for the other overload. I think it should be solved for the overload in line 116 too.

[diogocanut]

Hmm, what's your suggestion for implementing method on line 116 with our new implementation of javaSecuritySecureRandom?

[djspiewak]

This suddenly introduces an implicit Random[IO], which doesn't exist right now, and means that SecureRandom will end up being the de facto Random used in most situations. Is that what we want?

[armanbilge]

Oh, good catch, I totally missed this because of the inheritance relationship, and maybe this is what @durban was getting at in #3647 (comment).

Well, it's tricky. To be honest I'm not entirely sure if I see the harm, but these sorts of implicits have bitten us in the past. In fact, UUIDGen being an implicit is biting us already ...

[durban]

I didn't think about this when I wrote that coment, but I agree that it could be a problem. Although, the worst problem I can think of right now is that the SecureRandom is probably slower than a non-secure Random, which is not the end of the world... but it is still not clear to me how Random is expected to be used: a user creates an instance themselves, then passes that instance implicitly?

[armanbilge]

Yeah it's not the end of the world. Much better to default to SecureRandom than the other way around lol ...

but it is still not clear to me how Random is expected to be used: a user creates an instance themselves, then passes that instance implicitly?

See also:

• The Random docs skip over the hardest part ..and other doubts  #3519

[durban]

A general comment: does this PR contain copied code from http4s? If yes, maybe that should be mentioned somewhere...

outdated

[djspiewak]

Conflicts need to be resolved but otherwise g2g