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

Show:
Saturnino Abril
April 20, 2020, 1:08 PM

Previous test was made for v5.22. This time is passed for v5.19 (ESR).

Saturnino Abril
April 1, 2020, 2:44 PM

Tested and passed.

Patryk Pomykalski
January 30, 2020, 6:00 PM

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.

 

Agniva De Sarker
January 30, 2020, 5:39 PM

- 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.

Mario de Frutos
January 30, 2020, 5:18 PM

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?

Done

Mana

None

Assignee

Claudio Costa

QA Assignee

Saturnino Abril

Reporter

Ben Schumacher

Epic Link

Fix versions

Mattermost Team

Platform

Sprint

None

QA Testing Areas

Messages

GitHub Issue

None

Components

None

Severity

S1_data_loss_or_crash