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.

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

proposal: math/rand: add Reader variable #16776

Closed
dsnet opened this issue Aug 17, 2016 · 8 comments
Closed

proposal: math/rand: add Reader variable #16776

dsnet opened this issue Aug 17, 2016 · 8 comments

Comments

@dsnet
Copy link
Member

dsnet commented Aug 17, 2016

The crypto/rand has both a Read function and a Reader object for passing in as an io.Reader. The math/rand package only has a Read function and no Reader object.

Should we add a Reader to math/rand to make it consistent with crypto/rand?

@dsnet dsnet added this to the Unplanned milestone Aug 17, 2016
@bradfitz
Copy link
Contributor

I thought this was a dup, but I was thinking of #8330 which went most of the way, but not all the way.

I still think there was an open bug or thread about this, but I don't remember the outcome.

@dsnet
Copy link
Member Author

dsnet commented Aug 17, 2016

When #8330 was first implemented, I noticed that it didn't include an implementation for a Reader object. Back then, I didn't speak up because it's not a big deal.

I think it'd be nice, but everytime I really wanted to use random Reader, I just do rand.New(rand.NewSource(0)) and move on in life.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 14, 2019
@ldemailly
Copy link

It would be nice, in particular to provide a concurrent safe version of rand.New(rand.NewSource(0)) that can be used for instance for github.com/google/uuid.SetRand() for fast concurrent uuid generation

@rsc rsc changed the title math/rand: add Reader variable proposal: math/rand: add Reader variable Sep 7, 2022
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 7, 2022
@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

Bumping over to proposal process. I think we should not do this. There are very few reasons to use a pseudo-random byte stream, and I worry about 'rand.Reader' completing to math/rand.Reader instead of crypto/rand.Reader. In general I wish we'd done a better job of keeping math/rand and crypto/rand's APIs completely disjoint, and in particular I regret adding math/rand.Read in the first place. I don't think we should exacerbate the problem by adding math/rand.Reader as well.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@ldemailly
Copy link

unfortunate but for whoever gets to this issue while looking for a concrete solution:

here is the workaround

// SyncReader is a thread-safe wrapper for a reader.
type SyncReader struct {
	lk     sync.Mutex
	reader io.Reader
}

// NewSyncReader returns a new thread-safe reader.
func NewSyncReader(reader io.Reader) *SyncReader {
	return &SyncReader{
		reader: reader,
	}
}

func (r *SyncReader) Read(p []byte) (n int, err error) {
	r.lk.Lock()
	defer r.lk.Unlock()

	return r.reader.Read(p)
}

// Then you can use:

rander = NewSyncReader(rand.New(rand.NewSource(time.Now().UnixNano()))) //nolint:gosec // we want fast not crypto

uuid.Must(uuid.NewRandomFromReader(rander)).String()

https://github.com/fortio/fortio/blob/v1.38.0/fhttp/http_utils.go#L603-L621

https://github.com/fortio/fortio/blob/v1.38.0/fhttp/http_client.go#L66

https://github.com/fortio/fortio/blob/v1.38.0/fhttp/http_client.go#L1075

@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Sep 28, 2022
@golang golang locked and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants