Post.Props is not safe for concurrent R/W
Description
A test run on circleci failed with a panic stating a "concurrent map read and map write" for Post.Props. Heads up: I'm fairly confident that the code change is not related to the panic. (PR)
I'm unsure why the concurrent R/W happend, but I think it should be investigated and made sure that this panic can't happen in production.
Panic log:
QA Test Steps
A smoke test around post should be sufficient.
Activity
Previous test was made for v5.22. This time is passed for v5.19 (ESR).
Tested and passed.
Ideally before you give the object to another routine but I see that notification code is quite complicated and it would be hard to keep it correct when making bigger changes.
- Ah I didn’t know there were generators for deep copy. Would you be open to start a thread on the developers:server channel regarding this ? I would like to hear what others have to say about this.
Adding getter and mutex fixes it:
But taking a look at it is not a quick fix, is something deeper.
I’ve found some interesting things investigating a bit:
It’d be nice not to export props but for what I know in the serialization wouldn’t appear so if we have it exported then everyone could modify props.
Our code is relaying on having access to buil-in maps props (value, ok := …) being ok and optional value but if we use functions we can’t make ok value optional so we have to change our code there too
We have to return StringInterface because our code relies on it. For example here, here and here
As you said we have to decide how are we going to tackle this because we also have other models that have to be changed like for example Channel.
I have a question. When you say the solution is to use deep copies of the object where do you visualize to do the copy?