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
Comments
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.". |
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. |
@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.
@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. |
@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 |
Is it? https://golang.org/ref/spec#Index_expressions - it has different runtime semantics, only the syntax is similar.
It does: every type holds its own unique thread safety guarantees. |
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. |
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. |
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. |
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? |
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.
|
Change https://golang.org/cl/88515 mentions this issue: |
@pciet SGTM, thanks. Sent a CL. |
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
The text was updated successfully, but these errors were encountered: