versioning flakes pt2 #7473
versioning flakes pt2 #7473
Conversation
|
|
||
| func (l *withLogger) prependTags(tags []tag.Tag) []tag.Tag { | ||
| return append(l.tags, tags...) | ||
| allTags := make([]tag.Tag, len(l.tags)+len(tags)) |
There was a problem hiding this comment.
pasting this for a reader from the future: this function was being accessed by multiple go-routines and was resulting in a race condition. append becomes a read+write operation when the underlying array for the slice has it's length reached the slice's max-capacity. This results in a new slice being made (hence a write operation) leading to potential data-races.
For a better, more detailed explanation: https://medium.com/@cep21/gos-append-is-not-always-thread-safe-a3034db7975
| copy(allTags, l.tags) | ||
| copy(allTags[len(l.tags):], tags) |
There was a problem hiding this comment.
Is it possible that the l.tags changed (like expanded) after allTags is created, will that cause trouble as allTags may not be big enough to hold l.tags?
There was a problem hiding this comment.
Looking at the code now, I don't think so - the reason being, this change effectively removes appending to l.tags without making a new withLogger instance - in other words, l.tags will grow when someone adds new tags to it which happens in WithLogger.With. However, WithLogger.With returns a new instance of the logger which shall have a new instance of l.tags
What changed?
fixed some code to reduce versioning flakes by making the logger library thread-safe to avoid data race, and one test with a context timeout issue:
TestVersioning3FunctionalSuite/TestUnpinnedWorkflowWithRamp_ToUnversioned (with_logger data-race ✅)
TestWorkerDeploymentSuite/TestSetWorkerDeploymentRampingVersion_Unversioned_VersionedCurrent ( context timeout - maybe fixed by having a separate go-routine ✅ )
TestDeploymentVersionSuite/TestDrainageStatus_SetCurrentVersion_YesOpenWFs (with_logger data-race ✅)
TestVersioningFunctionalSuite/TestDispatchActivityFailCrossTq (retry 2) (with_logger data-race ✅)
Why?
How did you test it?
-raceand now see no errors (errored out previously)Potential risks
Documentation
Is hotfix candidate?