Skip to content

Commit 7ef9914

Browse files
authored
Fix TSAN violations in FSTTransactionTests (#5922)
TSAN uncovered cases where we were reading values in the transaction thread that were set on the main thread and were unprotected by lock. In practice this is unlikely to cause an harm, but it's good to keep TSAN clean for when there are real issues.
1 parent d617e99 commit 7ef9914

File tree

1 file changed

+18
-17
lines changed

1 file changed

+18
-17
lines changed

Firestore/Example/Tests/Integration/FSTTransactionTests.mm

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,14 @@ - (FSTTransactionTester *)runWithStages:(NSArray<TransactionStage> *)stages;
119119
- (void)expectDoc:(NSObject *)expected;
120120
- (void)expectNoDoc;
121121
- (void)expectError:(FIRFirestoreErrorCode)expected;
122+
123+
@property(atomic, strong, readwrite) NSArray<TransactionStage> *stages;
124+
@property(atomic, strong, readwrite) FIRDocumentReference *docRef;
125+
@property(atomic, assign, readwrite) BOOL fromExistingDoc;
122126
@end
123127

124128
@implementation FSTTransactionTester {
125129
FIRFirestore *_db;
126-
FIRDocumentReference *_docRef;
127-
BOOL _fromExistingDoc;
128-
NSArray<TransactionStage> *_stages;
129130
FSTTransactionTests *_testCase;
130131
NSMutableArray<XCTestExpectation *> *_testExpectations;
131132
}
@@ -141,17 +142,17 @@ - (instancetype)initWithDb:(FIRFirestore *)db testCase:(FSTTransactionTests *)te
141142
}
142143

143144
- (FSTTransactionTester *)withExistingDoc {
144-
_fromExistingDoc = YES;
145+
self.fromExistingDoc = YES;
145146
return self;
146147
}
147148

148149
- (FSTTransactionTester *)withNonexistentDoc {
149-
_fromExistingDoc = NO;
150+
self.fromExistingDoc = NO;
150151
return self;
151152
}
152153

153154
- (FSTTransactionTester *)runWithStages:(NSArray<TransactionStage> *)stages {
154-
_stages = stages;
155+
self.stages = stages;
155156
return self;
156157
}
157158

@@ -160,7 +161,7 @@ - (void)expectDoc:(NSObject *)expected {
160161
[self runSuccessfulTransaction];
161162

162163
XCTestExpectation *expectation = [_testCase expectationWithDescription:@"expectDoc"];
163-
[_docRef getDocumentWithCompletion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
164+
[self.docRef getDocumentWithCompletion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
164165
[self->_testCase assertSnapshot:snapshot equalsObject:expected error:error];
165166
[expectation fulfill];
166167
}];
@@ -174,7 +175,7 @@ - (void)expectNoDoc {
174175
[self runSuccessfulTransaction];
175176

176177
XCTestExpectation *expectation = [_testCase expectationWithDescription:@"expectNoDoc"];
177-
[_docRef getDocumentWithCompletion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
178+
[self.docRef getDocumentWithCompletion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
178179
[self->_testCase assertDoesNotExistWithSnapshot:snapshot error:error];
179180
[expectation fulfill];
180181
}];
@@ -191,9 +192,9 @@ - (void)expectError:(FIRFirestoreErrorCode)expected {
191192
}
192193

193194
- (void)prepareDoc {
194-
_docRef = [[_db collectionWithPath:@"nonexistent"] documentWithAutoID];
195+
self.docRef = [[_db collectionWithPath:@"nonexistent"] documentWithAutoID];
195196
if (_fromExistingDoc) {
196-
NSError *setError = [self writeDocumentRef:_docRef data:@{@"foo" : @"bar"}];
197+
NSError *setError = [self writeDocumentRef:self.docRef data:@{@"foo" : @"bar"}];
197198
NSString *message = [NSString stringWithFormat:@"Failed set at %@", [self stageNames]];
198199
[_testCase assertNilError:setError message:message];
199200
}
@@ -217,8 +218,8 @@ - (void)runSuccessfulTransaction {
217218
[_testCase expectationWithDescription:@"runSuccessfulTransaction"];
218219
[_db
219220
runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) {
220-
for (TransactionStage stage in self->_stages) {
221-
stage(transaction, self->_docRef);
221+
for (TransactionStage stage in self.stages) {
222+
stage(transaction, self.docRef);
222223
}
223224
return @YES;
224225
}
@@ -239,8 +240,8 @@ - (void)runFailingTransactionWithError:(FIRFirestoreErrorCode)expected {
239240
[_testCase expectationWithDescription:@"runFailingTransactionWithError"];
240241
[_db
241242
runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) {
242-
for (TransactionStage stage in self->_stages) {
243-
stage(transaction, self->_docRef);
243+
for (TransactionStage stage in self.stages) {
244+
stage(transaction, self.docRef);
244245
}
245246
return @YES;
246247
}
@@ -256,14 +257,14 @@ - (void)runFailingTransactionWithError:(FIRFirestoreErrorCode)expected {
256257
}
257258

258259
- (void)cleanupTester {
259-
_stages = [NSArray array];
260+
self.stages = [NSArray array];
260261
// Set the docRef to something else to lose the original reference.
261-
_docRef = [[self->_db collectionWithPath:@"reset"] documentWithAutoID];
262+
self.docRef = [[self->_db collectionWithPath:@"reset"] documentWithAutoID];
262263
}
263264

264265
- (NSString *)stageNames {
265266
NSMutableArray<NSString *> *seqList = [NSMutableArray array];
266-
for (TransactionStage stage in _stages) {
267+
for (TransactionStage stage in self.stages) {
267268
if (stage == delete1) {
268269
[seqList addObject:@"delete"];
269270
} else if (stage == update1 || stage == update2) {

0 commit comments

Comments
 (0)