singleflight: Add an example #73

kevinburke · 2016-10-23T06:17:02Z

I was curious about the best way to initialize a Group - it turns out you just
do var g Group - but figured this package could use a package-level example
demonstrating an example use case.

I've signed the CLA.

bradfitz · 2016-10-23T13:36:01Z

singleflight/example_test.go

+		wg.Add(1)
+		go func(i int) {
+			ctrval, _ := g.Do("key", func() (interface{}, error) {
+				counter++


I don't like how this example basically teaches people how to write data races. You have to already understand how singleflight works to understand why this perhaps is safe.

Use atomic.AddInt32 instead? And then put a same-line // atomic comment on the var counter int32 // atomic line.

bradfitz · 2016-10-23T13:36:08Z

singleflight/example_test.go

@@ -0,0 +1,37 @@
+package singleflight


Missing copyright header.

bradfitz · 2016-10-23T13:36:57Z

singleflight/example_test.go

+	}
+	// Sleep so all of the goroutines start before we send a message on the
+	// channel
+	time.Sleep(10 * time.Millisecond)


I'm never a fan of sleeps in tests. Use a channel or something to wait for everybody to get in there instead.

I played around with this but I don't think it's possible. Thread 1 enters the function body, but I don't think threads 2-5 have a way to broadcast to thread 1 that they have also entered the Do() and are waiting for the results. The test for this functionality also uses a sleep. :(

I think the example is getting a little too complex, so I got rid of the threads and just added a comment explaining the behavior.

bradfitz · 2016-10-25T04:52:34Z

singleflight/example_test.go

+}
+
+func Example() {
+	var g singleflight.Group


This is generally a global variable. With this style of example (func Example), it will show the whole file, right? So I think you can move this out? Then you'd want to name it something better than "g".

kevinburke · 2016-10-26T18:51:05Z

Here's how the example looks on godoc on my machine:

bradfitz · 2016-10-26T20:54:58Z

Check errors too.

kevinburke · 2016-10-28T05:32:53Z

Done. Thank you for the review! And sorry for being so slow.

$ git rev-parse HEAD && go test ./...
8340abafe7a8d6b5443f502e62eb1d8e16198d52
ok      github.com/golang/groupcache    0.691s
ok      github.com/golang/groupcache/consistenthash 0.011s
?       github.com/golang/groupcache/groupcachepb   [no test files]
ok      github.com/golang/groupcache/lru    0.012s
ok      github.com/golang/groupcache/singleflight   0.119s
?       github.com/golang/groupcache/testpb [no test files]

bradfitz · 2016-10-29T17:15:59Z

Copyright Google isn't right. You're not a Google employee I don't think?

I'd make it match the rest of Go and be copyright The Go Authors.

kevinburke · 2016-10-29T17:25:22Z

Ah, sorry, I copied it from another file in that repo. Should I update the
other file headers or just this one?

On Saturday, October 29, 2016, Brad Fitzpatrick [email protected]
wrote:

Copyright Google isn't right. You're not a Google employee I don't think?

I'd make it match the rest of Go and be copyright The Go Authors.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#73 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAOSI_80N7g_J8gylOG7N65zuy3FtDVoks5q439egaJpZM4KeD5v
.

Kevin Burke
925.271.7005 | kev.inburke.com

bradfitz · 2016-10-29T18:07:07Z

Just one for now. Once a non-Googler touches other files they can update it.

kevinburke · 2016-10-30T18:19:40Z

Done

bradfitz · 2016-11-14T20:37:57Z

singleflight/example_test.go

+func expensiveNetworkCall() (interface{}, error) {
+	time.Sleep(1)
+	atomic.AddInt32(&counter, 1)
+	return counter, nil


This is a data race. You're reading from counter without atomics.

Yikes. Should be fixed now. Thanks for sticking with this through several rounds.

hblanks · 2016-12-22T15:28:07Z

Looks like you still have that data race to fix, @kevinburke :-)

kevinburke · 2016-12-22T15:31:07Z

Yikes. I think I forgot to push my branch months ago when I left this comment. Let me double check

…

On Thu, Dec 22, 2016 at 07:28 Hunter Blanks ***@***.***> wrote: Looks like you still have that data race to fix, @kevinburke <https://github.com/kevinburke> :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#73 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOSI6xQFVCqigIGlirJsqVwTgCfC0r-ks5rKpcOgaJpZM4KeD5v> .

bradfitz · 2017-02-24T20:41:49Z

singleflight/example_test.go

+}
+
+func Example() {
+	// No matter how many threads call Do, only one network call per key will


no "threads"

bradfitz · 2017-02-24T20:43:37Z

singleflight/example_test.go

+	// No matter how many threads call Do, only one network call per key will
+	// be in progress at any time. Callers that share a key will get the same
+	// results as an in-flight call with that key.
+	v, err := networkGroup.Do("key", expensiveNetworkCall)


Normally this expensiveNetworkCall is a closure, closing over the key value. Maybe show an addition of +1 instead and say:

// Imagine this is something expensive: return n + 1, nil

And make your "key" be:

n := 41 group.Do(strconv.Itoa(n), func(...) {..

(no need to mention 'network')

kevinburke · 2018-03-26T22:00:03Z

Apologies for the delay, but I've finally made the changes you requested.

I was curious about the best way to initialize a Group - it turns out you just do `var g Group` - but figured this package could use a package-level example demonstrating an example use case.

mcspring · 2019-01-09T10:47:08Z

singleflight/example_test.go

+var counter int32
+var group singleflight.Group
+
+func Example() {


Should this be a testing case, like TestSingleFlightExample, and use testing.T for checking?

bradfitz suggested changes Oct 23, 2016

View reviewed changes

kevinburke force-pushed the singleflight-example branch from 5ea4293 to ef32c4f Compare October 24, 2016 00:42

bradfitz reviewed Oct 25, 2016

View reviewed changes

kevinburke force-pushed the singleflight-example branch from ef32c4f to f3f9685 Compare October 26, 2016 18:48

kevinburke force-pushed the singleflight-example branch from f3f9685 to f63e6c5 Compare October 26, 2016 18:51

kevinburke force-pushed the singleflight-example branch from f63e6c5 to 8340aba Compare October 28, 2016 05:31

kevinburke force-pushed the singleflight-example branch from 8340aba to fdb9e97 Compare October 29, 2016 18:11

bradfitz reviewed Nov 14, 2016

View reviewed changes

kevinburke force-pushed the singleflight-example branch from fdb9e97 to 5680294 Compare November 25, 2016 19:18

googlebot added the cla: yes label Feb 24, 2017

bradfitz reviewed Feb 24, 2017

View reviewed changes

kevinburke force-pushed the singleflight-example branch from 5680294 to f1fb5e5 Compare March 26, 2018 21:58

singleflight: Add an example

91138e8

I was curious about the best way to initialize a Group - it turns out you just do `var g Group` - but figured this package could use a package-level example demonstrating an example use case.

kevinburke force-pushed the singleflight-example branch from f1fb5e5 to 91138e8 Compare May 13, 2018 07:21

mcspring suggested changes Jan 9, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

singleflight: Add an example #73

singleflight: Add an example #73

kevinburke commented Oct 23, 2016

bradfitz Oct 23, 2016

kevinburke Oct 24, 2016

bradfitz Oct 23, 2016

kevinburke Oct 24, 2016

bradfitz Oct 23, 2016

kevinburke Oct 24, 2016

bradfitz Oct 25, 2016

kevinburke Oct 26, 2016

kevinburke commented Oct 26, 2016

bradfitz commented Oct 26, 2016

kevinburke commented Oct 28, 2016

bradfitz commented Oct 29, 2016

kevinburke commented Oct 29, 2016

bradfitz commented Oct 29, 2016

kevinburke commented Oct 30, 2016

bradfitz Nov 14, 2016

kevinburke Nov 25, 2016

hblanks commented Dec 22, 2016

kevinburke commented Dec 22, 2016 via email

bradfitz Feb 24, 2017

bradfitz Feb 24, 2017

kevinburke commented Mar 26, 2018

mcspring Jan 9, 2019

singleflight: Add an example #73

Are you sure you want to change the base?

singleflight: Add an example #73

Conversation

kevinburke commented Oct 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinburke commented Oct 26, 2016

bradfitz commented Oct 26, 2016

kevinburke commented Oct 28, 2016

bradfitz commented Oct 29, 2016

kevinburke commented Oct 29, 2016

bradfitz commented Oct 29, 2016

kevinburke commented Oct 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hblanks commented Dec 22, 2016

kevinburke commented Dec 22, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinburke commented Mar 26, 2018

Choose a reason for hiding this comment