-
-
Notifications
You must be signed in to change notification settings - Fork 654
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
Enable the race detector on Sytest tests #745
Conversation
It turns out the races are in the tests - #748. Maybe we can merge that first and drop the soft fail? Perhaps also enable the race detector when running SyTest? |
I wonder where we're actually going to see race conditions brought out by sytest... |
Removed the soft-fail and moving the pipeline bits to a PR on https://github.com/matrix-org/pipelines |
Moved unit test race stuff to matrix-org/pipelines#2 I think we may want to wait on this one until we fix the races that Sytest brings up, otherwise we'll suffer from much slower CI. |
Race condition found in sytest: #780 |
What's the status of this @anoadragon453 ? I'd obviously like to have race detection for sytests - what is the performance hit if we enabled this by default? |
IIRC it did have a noticeable performance hit, but I'd try it again and see if things have improved in the newer go versions. The race conditions mentioned in this issue were fixed but more may have cropped up with code change. Feel free to take this and:
You'll want to fix any existing race conditions before merging the option to CI ofc. |
On my local machine, running a full SyTest test with race detector on uses ~2x time. Good thing is that I don't see any race conditions in the output. It seems this line in SyTest needs to be removed in order to see the race condition warnings in the output: https://github.com/matrix-org/sytest/blob/034199da24c60014751503a059fe1ea7ac757381/lib/SyTest/Homeserver/ProcessManager.pm#L112 |
I'm going to close this for now because a 2x hit on speed is pretty bad. We already feel the pain having it be 17 minutes long, so bumping it to 34 minutes is unacceptable. For now, let's manually and periodically run the race detector. |
Enable the go race detector.
Upon doing so, I found we indeed have race conditions!
Race conditions
The test cases have been allowed to fail now, but separate PRs should be made to fix these race conditions, such that we can later drop the soft fail again.
Fixes #491