Skip to content
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.

Sign up for GitHub

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

Jump to bottom

proposal: x/exp/rand: Make LockedSource zero value useful #49342

Closed
Merovius opened this issue Nov 4, 2021 · 4 comments
Closed

proposal: x/exp/rand: Make LockedSource zero value useful #49342

Merovius opened this issue Nov 4, 2021 · 4 comments
Labels
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@Merovius
Copy link
Contributor

Merovius commented Nov 4, 2021

Previously, #25988 was put on hold, because API changes to x/exp/rand where hold off on. I'm running into the same problem right now. I would assume that over the last three years the decision of whether or not we want to change math/rand for Go 2 has manifested a bit more.

My proposal is less of an API change. Instead of adding a NewLockedSource function, I'd suggest to change LockedSource.src from a pointer into a value. The methods on LockedSource have pointer-receivers, so we'd still be able to call pointer-methods on it. This would both make src.Seed work on a zero-value LockedSource and remove a pointer indirection.

The reasons I want to use a LockedSource are the same as @krancour mentions in the above issue:

The standard library puts us between a rock and a hard place, forcing us to choose:

  1. Concurrency safety (by using math/rand package level functions), but risk the shared Source being stupidly re-seeded by other code with a value like 1.
  2. A seeded Source that you know for sure hasn't been / cannot be stupidly re-seeded by other code (i.e. not math/rand package level Source), but is not concurrency safe.

That is exactly the trade-off I've run into a couple of times now. For now, I'll probably use a package-scoped sync.Pool, but it would be great if we could just get a package-scoped, concurrency-safe *rand.Rand.

Also, I think if we don't add either a constructor or do something like this proposal, we should remove LockedSource again. There is no benefit to exporting a type which can't be used.
@gopherbot gopherbot added this to the Proposal milestone Nov 4, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Nov 4, 2021
@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

/cc @robpike

@robpike robpike self-assigned this Nov 11, 2021
@robpike
Copy link
Contributor

robpike commented Feb 11, 2022

This seems good to me. I'm happy to accept a change.

@gopherbot
Copy link

Change https://go.dev/cl/385094 mentions this issue: rand: make zero LockedSource useful

@Merovius
Copy link
Contributor Author

Merovius commented Feb 11, 2022 •
edited

Sent a CL.

FWIW, I don't know what the plans for x/exp/rand are right now, but I think if it ever gets promoted to the stdlib, the LockedSource API should get a second look. Specifically

  • The way it is set up right now means you can't have a LockedSource which isn't based on PCGSource.
  • The concurrency-safety of Rand depends on the type-assertion on *LockedSource. IMO this weakens the abstraction of Source.
  • It seems strange to me that (*LockedSource).Read is exported and (*LockedSource).seedPos isn't - both are the same kind of implementation-detail.

I suggested this solution, because #25988 was put on hold for changing the API, so I didn't want to do that. But IMO better options would be (in order of preference)

  1. Unexport type LockedSource and add func LockedSource(Source) Source. This is akin to io.LimitReader etc.
  2. Add func NewLockedSource(src Source) *LockedSource. Maybe unexport (*LockedSource).Read or export (*LockedSource).seedPos.
  3. Unexport LockedSource, remove the package-scoped Seed function. Instead, choose a good seed in init and re-seed automatically every once in a while. Users can use package-scoped functions to "just get some random values" or use a non-concurrency-safe *rand.Rand for finer control.

I bring this up now, because currently no program actually uses *LockedSource, so arguably unexporting it again isn't much of a breaking change. But mainly, I don't want us to commit to a bad API at some point in the future.

@rsc rsc unassigned robpike Jun 23, 2022
@seankhliao seankhliao removed this from Incoming in Proposals (old) Jul 3, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
4 participants
@rsc @Merovius @robpike @gopherbot