Increase thread safety on variables - #592
Conversation
…urrent writes. Appending to a slice is not thread-safe. The actual value is safe, but the header for the slice is not. Protect it with a Mutex. This will allow more liberal use of t.Parallel() without data races using RegisterUser(), LoginUser() and Client() to access users on a Homeserver under test when sharing a deployment.
…cessed concurrently, mainly by SendEventUnsynced. Make it atomic(fix it in the other places it is used as well).
…d-safe parts, State and Timeline. Protect them with a RWMutex, as most of their usages are reads. This touched the SendJoinRequestHandler as well, as it reads from the State map.
…a data race. Protect it with atomics.
…hat was causing a data race. Protect it with atomics.
…ct it with a RWMutex as RegisterUser() can write concurrently when using t.Parallel().
…read-safe by using atomics.
|
Thanks a lot for this PR, some flakes have been annoying us for a while, like We think your changes make sense, but we also think that having to think about that kind of stuffs in a testing framework is... really annoying :) As a first, could you split out the part with the changes on the We are going to put a bit more thinking about that in |
|
This mostly touches Complement internals, so I'm taking it off the Synapse team's review queue. |
kegsay
left a comment
kegsay
left a comment
There was a problem hiding this comment.
The internal package changes are very sane, thanks!
Complement internals are generally not thread-safe to use, but yes in practice we should make them (specifically the Deployment/Server/ServerRoom).
|
For context we also had a lengthy discussion on should we really use In the meantime, thanks a lot for that @realtyem , less flakes ! |
4 participants
While experimenting with Complement and trying to speed up a few things, I noticed some race conditions causing inconsistent results. I started tracking them down one-by-one using the
-raceflag ongo test. Fixing these allows heavier usage oft.Parallel()during tests while sharing a deployment and registering fresh users withRegisterUser()(ala, federation_room_join_partial_state_test.go and #471) and it's siblings and derivatives. This alone will not(unfortunately) speed up Complement on Synapse(especially on Github CI), but will pave the way for less odd and inconsistent errors in the future. Hopefully, some flakes will go away as well(looking at youClient Timeout while awaiting headers).Mutex's and RWMutex's were the main go-to, but atomic.AddInt64() was also used in a few places where I felt it was more appropriate.
There is an entire set of data races I couldn't touch from this repo, those caused by gorilla's mux package. The problem stems from the complement test server that is created to test federation responses. Sometimes a test server is created, then started with
Listen(), and then an additional Handler is added on to the server(which is necessary to get the results of the test). This causes a data race in the Router.routes slice when Router.Match is concurrently called. Unfortunately, this package is no longer maintained and has been archived. If someone at matrix.org would like to fork it, I can submit the PR to patch it. I'm still exploring other packages that might meet the requirements as a replacement, but would appreciate suggestions or a fuller list of said requirements than I can get by just reading the source code.Should be reviewable commit-wise.
Useful links:
Signed-off-by: Jason Little realtyem@gmail.com