code review 137350043: sync/once: panicking functions still marked as complete

sync/once: panicking functions still marked as complete

This is a corner case, and one that was even tested, but this
CL changes the behavior to say that f is "complete" even if it panics.
But don't think of it that way, think of it as sync.Once runs
the function only the first time it is called, rather than
repeatedly until a run of the function completes.

Fixes issue 8118.

[r, 2014-09-08 19:44:32]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[r, 2014-09-08 20:09:10]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[r, 2014-09-09 18:49:31]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[r, 2014-09-16 20:42:40]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[dvyukov, 2014-09-16 20:57:51]

LGTM

[r, 2014-09-16 20:59:40]

*** Submitted as https://code.google.com/p/go/source/detail?r=e83e04e2de8a ***

sync/once: panicking functions still marked as complete

This is a corner case, and one that was even tested, but this
CL changes the behavior to say that f is "complete" even if it panics.
But don't think of it that way, think of it as sync.Once runs
the function only the first time it is called, rather than
repeatedly until a run of the function completes.

Fixes issue 8118.

LGTM=dvyukov
R=golang-codereviews, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/137350043

[josharian, 2014-09-16 21:09:50]

LGTM with nits

https://codereview.appspot.com/137350043/diff/60001/src/sync/once_test.go
File src/sync/once_test.go (right):

https://codereview.appspot.com/137350043/diff/60001/src/sync/once_test.go#new...
src/sync/once_test.go:42: func TestOncePanic(t *testing.T) {
This could be simpler now:

func TestOncePanic(t *testing.T) {
	var once Once
	func() {
		defer func() {
			if r := recover(); r == nil {
				t.Fatalf("Once.Do did not panic")
			}
		}()
		once.Do(func() {
			panic("failed")
		})
	}()

	once.Do(func() {
		t.Fatalf("Once.Do called twice")
	})
}

https://codereview.appspot.com/137350043/diff/80001/src/sync/once.go
File src/sync/once.go (right):

https://codereview.appspot.com/137350043/diff/80001/src/sync/once.go#newcode32
src/sync/once.go:32: // If f panics, Do considers it to have returned; future
calls of Do return
Maybe s/of Do //

[r, 2014-09-16 21:12:25]

https://codereview.appspot.com/137350043/diff/80001/src/sync/once.go
File src/sync/once.go (right):

https://codereview.appspot.com/137350043/diff/80001/src/sync/once.go#newcode32
src/sync/once.go:32: // If f panics, Do considers it to have returned; future
calls of Do return
On 2014/09/16 21:09:50, josharian wrote:
> Maybe s/of Do //

I wanted to be clear it's Do and not f we're talking about here. Also you
rewrite becomes

Future calls return without calling f, which is bizarre phraseology.

[r, 2014-09-16 21:13:04]

https://codereview.appspot.com/137350043/diff/60001/src/sync/once_test.go
File src/sync/once_test.go (right):

https://codereview.appspot.com/137350043/diff/60001/src/sync/once_test.go#new...
src/sync/once_test.go:42: func TestOncePanic(t *testing.T) {
Nice. Send a CL please?