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

ref/mem: misleading sentence in sync.Once example #27808

Closed
seebs opened this issue Sep 21, 2018 · 6 comments
Closed

ref/mem: misleading sentence in sync.Once example #27808

seebs opened this issue Sep 21, 2018 · 6 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@seebs
Copy link
Contributor

seebs commented Sep 21, 2018

What version of Go are you using (go version)?

1.11/playground

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

linux/amd64, playground

What did you do?

https://play.golang.org/p/w-CNHDQAGW-

What did you expect to see?

according to ref/mem as written, "1\n1 hello, 1\n2\n 2 hello 1\n"

What did you see instead?

the other way around

Observation: There's two things here that I think are potentially misleading, although both are sort of trivial. The first is that, according to the "goroutine destruction" section, there's no guarantee that anything gets printed, because nothing is waiting on these, and indeed, without the extra channel operations, nothing happens.

The second is the distinction between "the first call to doprint()", "the first call to doprint() which actually starts execution", and "the first call to doprint() which actually begins the once.Do() call". While in the trivial example, probably the first call to start executing will win that, it's not generally a given that two goroutines acting in parallel will always complete in the same order they start, even running identical code. But in particular, it's definitely not guaranteed that the "first call" will start executing first.

I'm fine with handwaving the synchronization thing, but I think the wording on "the first call" might benefit from cleaning up. Proposed wording:

The first call to doprint() to begin executing the once.Do() calls setup(), any calls that reach that point later do not, regardless of the order in which the goroutines are invoked or begin executing.

@bcmills
Copy link
Contributor

bcmills commented Sep 22, 2018

without the extra channel operations, nothing happens.

The channel is not necessary to the example: the goroutines are scheduled eventually, they're just not guaranteed to schedule before the program exits (if it does exit).
Contrast https://play.golang.org/p/XRbNXahEoY7.

it's definitely not guaranteed that the "first call" will start executing first.

The “first call” in the context of that sentence is the first call that “happens” in the happens-before relationship. There is not well-defined “first call” to sync.Once.Do itself. (The call of the function setup is what has a well-defined order, not the call of the method Do.)

You're right that the sentence “The first call to doprint runs setup once.” is misleading, but it's also not the important sentence in that section: we should probably just delete it.

The important sentence in that section is:

A single call of f() from once.Do(f) happens (returns) before any call of once.Do(f) returns.

@bcmills
Copy link
Contributor

bcmills commented Sep 22, 2018

The first call to doprint() to begin executing the once.Do() calls setup(), any calls that reach that point later do not, regardless of the order in which the goroutines are invoked or begin executing.

As noted above: there is no “first call […] to begin executing the once.Do”, because the execution of once.Do itself is not ordered within the memory model: only the return from once.Do is ordered, and only with respect to the return from whichever function happened to be executed.

The important behavior of sync.Once is that the execution of exactly one function passed to Do returns before any call to Do on that sync.Once returns. If there is more than one candidate for such a function, the memory model does not specify which one wins: only that the one that does happens before any of the calls returns.

@bcmills
Copy link
Contributor

bcmills commented Sep 22, 2018

A clearer fix might be to change the sentence:

The first call to doprint runs setup once.

to

The call to setup completes before either call to print begins.”

@bcmills bcmills changed the title ref/mem contains subtle errors in sync.Once example ref/mem: misleading sentence in sync.Once example Sep 22, 2018
@bcmills bcmills added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Sep 22, 2018
@bcmills bcmills added this to the Go1.12 milestone Sep 22, 2018
@seebs
Copy link
Contributor Author

seebs commented Sep 22, 2018

That's a good point. I'm pondering this, because I think there's a mild ambiguity there, but on reflection, I think it's just the general ambiguity that nothing promises that code ever executes at all. For instance, so far as I can tell, if a hostile implementor looked at the sample code, and decided that they could prove that two things would eventually reach the once.Do(), they would be allowed to implement it so that the first goroutine to reach that point necessarily blocks, and the second is allowed to run the provided function. But I guess that doesn't actually matter at all, as long as they don't block indefinitely for no particular reason.

Hmm.

Come to think of it:

Multiple threads can execute once.Do(f) for a particular f, but only one will run f(), and the other calls block until f() has returned.

Shouldn't that be "for a particular once"?

	switch(i) {
	case 1: once.Do(one)
	case 2: once.Do(two)
	}

@seebs
Copy link
Contributor Author

seebs commented Sep 22, 2018

On reading docs and trying it: It clearly should, given that the docs for sync say "if Do is being called for the first time for this instance of Once", and further clarify "regardless of the value of f". So they talk about it in terms of the first call to Do(), not the first invocation of f. That's probably important because it's possible for f to vary. Indeed, the docs suggest that you may need a local function literal, and so far as I can tell, even if it's the same source code text each time, each time you reach a function literal, you have a logically distinct "function" -- it's a closure, and will be bound to a different local environment each time.

@gopherbot
Copy link

Change https://golang.org/cl/155637 mentions this issue: doc: go_mem: clarify Once docs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants