Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make
UUIDGen
more performant #3647Make
UUIDGen
more performant #3647Changes from 19 commits
dbd35db
22db811
2202a9f
cb28120
c7a167b
77d6081
6552d20
ab1573d
3d0d4c4
b0bf31c
47b3544
d906316
b611dda
1655b79
5a9259c
1c6b6ec
c4c6dc0
e98eb09
c2633ee
816d1c4
5519064
db8a428
201d8e0
4b4ded3
eead224
7764e24
31897c4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise it will create a new one each time :)
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
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how to add the implicit to scope having it as lazy val?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I meant to write
implicit lazy val
:)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@armanbilge What was the problem in FS2
Network
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theIOApp
s polling system into this globally promisedNetwork[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
orResource
by the user. It's some kind of UX trade-off.See a similar place where we faced this in:
Console#readLine
cancelable #3465There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, forSecureRandom
, 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 :-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for the best case
n: Int
. It will only sense in the worst case, when we fall back to a pool.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what's your suggestion for implementing method on line 116 with our new implementation of
javaSecuritySecureRandom
?