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

doc: unclear if maps are ok for concurrent reads #23480

Closed
gbbr opened this Issue Jan 18, 2018 · 12 comments

Comments

Projects
None yet
7 participants
@gbbr
Copy link
Member

gbbr commented Jan 18, 2018

It isn't specified anywhere in the docs if concurrent reads from a map (or any other type for that matter) are ok in Go. While this may be obvious to some, it would be good to put it somewhere.

Potential candidates:
https://golang.org/ref/mem
https://golang.org/doc/faq#atomic_maps

Some context:
https://groups.google.com/forum/m/#!topic/golang-nuts/3FVAs9dPR8k

@gbbr gbbr added the Documentation label Jan 18, 2018

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Jan 18, 2018

It is specified in the memory model that concurrent reads are allowed. Or at least, that fact is derivable from the memory model.

The memory model doesn't really define "read" and "write". It seems pretty obvious, though. I guess it wouldn't hurt to add a paragraph clarifying that for complex types. Maybe next to the text about "Reads and writes of values larger than a single machine word behave as multiple machine-word-sized operations in an unspecified order.".

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 18, 2018

I have met multiple people new-ish to Go that guarded map reads with locks, or used third-party concurrent maps for just concurrent reads. This definitely warrants a hint or a tip somewhere for beginners.

@zerkms

This comment has been minimized.

Copy link

zerkms commented Jan 18, 2018

@mvdan if nothing guarantees a read from a complex data structure to be thread safe - it's safe to assume it's not. Load from a map requires hashing, who knows if it involves some internal state operations or not.

It is specified in the memory model that concurrent reads are allowed

@randall77 it's also implied but really crucial and subtle that before initialisation there must be a "happens before" relation established still. It's often hard to spot, so just a "concurrent reads from a map are safe" in the MM/spec might be more dangerous than nothing at all.

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Jan 18, 2018

@mvdan: If that's the worst mistake a beginner makes, I'll call that a success. It's only a performance bug, after all.

I can certainly see how one might be unsure if map reads are in fact reads according to the memory model. I made exactly that mistake in my first change to the map code, assuming I can do some writes in the internals of a map read. But at the language spec level, a map read m[k] is just like a slice read s[i], the internal complexity doesn't really matter.

@zerkms

This comment has been minimized.

Copy link

zerkms commented Jan 19, 2018

But at the language spec level, a map read m[k] is just like a slice read s[i],

Is it? https://golang.org/ref/spec#Index_expressions - it has different runtime semantics, only the syntax is similar.

the internal complexity doesn't really matter.

It does: every type holds its own unique thread safety guarantees.

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Jan 19, 2018

Of course they have different semantics. One is a map, one is a slice. But they are both reads.

Every type does not have its own unique thread safety guarantees. The only things unique in that respect are channel and synchronization primitive ops. Everything else is either in one of two classes, a read or a write. And it isn't type based, it's syntax based (i.e. x = y is a write ofx and a read of y, regardless of the types of x and y).

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 19, 2018

Our usual pattern in Go is to document the standard behavior and the exception. A read from a map is just a read. A read from a slice is just a read. Concurrent reads are permitted. If reads from maps were not just reads, we would have to document that. But since they are, it's not really obvious whether or where to document it.

I understand that people who are familiar with other languages may puzzle over whether concurrent reads from maps are permitted. It's a natural way to overthink the problem. I'm not strongly opposed to adding something to the memory model doc, but since in general we don't want people to have to read that doc I'm not sure it will solve the problem, if there is indeed a problem.

Maybe a blog article would be a better approach.

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 19, 2018

A blog post could work - I found the slice internals one very useful and simple to follow early into my learning of Go.

I don't think adding information to the memory model would be useful. New people to the language are far more likely to read tutorials, blog posts, or the spec. I haven't even read the memory model, to give an example, but I have read all of the above.

@gbbr

This comment has been minimized.

Copy link
Member Author

gbbr commented Jan 19, 2018

I agree that perhaps the memory model is not the best place to put this since it doesn’t seem to be a document that is referenced a lot, nor are people encouraged to read it. Let’s not change that.

I do however find (by Googling around and looking at past history) that this discussion already happened quite a couple of times before, on the official channel and other places such as StackOverflow. For that reason I would like to suggest that perhaps a good place to add this would be the FAQ, since this seems to be indeed a frequently asked question. Does anyone see any issues with that?

I would of course consider the issue closed with a blog post, but for that someone would have to take this action and write it, plus I am not sure exactly what this post would cover - would it be meant to be along the same lines as the slice post?

@pciet

This comment has been minimized.

Copy link
Contributor

pciet commented Jan 19, 2018

I always assumed a RWMutex is fine for map access and read-only maps don’t need to be synchronized, but reading the faq I can see how somebody may think reads are also not concurrent safe. Adding a sentence to that faq makes sense to me.

The language does not preclude atomic map updates. When required, such as when hosting an untrusted program, the implementation could interlock map access. Maps with only read operations are safe to access concurrently.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 19, 2018

Change https://golang.org/cl/88515 mentions this issue: doc/faq: clarify that concurrent maps reads are safe

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 19, 2018

@pciet SGTM, thanks. Sent a CL.

@gopherbot gopherbot closed this in 984e81f Jan 20, 2018

@golang golang locked and limited conversation to collaborators Jan 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.