Skip to content

iOS: fix data race in FSTDispatchQueue. #1070

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 11, 2018
Merged

iOS: fix data race in FSTDispatchQueue. #1070

merged 7 commits into from
Apr 11, 2018

Conversation

var-const
Copy link
Contributor

operationInProgress is accessed from both the main thread and from
libdispatch on some other thread. Make it atomic to avoid a data race.

Tested: ran unit tests using old and new versions under Thread
Sanitizer, verified that TSan reports a data race for the old version,
but finds no issues with the new version.

@var-const
iOS: fix data race in FSTDispatchQueue.
e43d20f 
operationInProgress is accessed from both the main thread and from
libdispatch on some other thread. Make it atomic to avoid a data race.

Tested: ran unit tests using old and new versions under Thread
Sanitizer, verified that TSan reports a data race for the old version,
but finds no issues with the new version.
var-const
var-const commented Apr 11, 2018
View reviewed changes
Firestore/Source/Util/FSTDispatchQueue.mm Outdated
@@ -16,6 +16,8 @@

#import <Foundation/Foundation.h>

#import <atomic>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I just added checks to cpplint to help with that and it has called you out: https://travis-ci.org/firebase/firebase-ios-sdk/jobs/365187148#L641 :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I deleted the comment once I noticed, but you were quicker.

Firestore/Source/Util/FSTDispatchQueue.mm Outdated
@@ -145,20 +147,20 @@ - (void)markDone {

#pragma mark - FSTDispatchQueue

@interface FSTDispatchQueue ()
@interface FSTDispatchQueue () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add an instance variable here, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though customarily it's just done on the @implementation.

We use the empty category to make "private" declarations that must be in an @interface. Things like property declarations or a hidden designated initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it appears that if I move the variable declaration inside @implementation, it becomes a global variable, not instance variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The declaration must still be inside braces. See e.g.

https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Local/FSTLevelDBKey.mm#L471

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the explanation. Done.

Firestore/Source/Util/FSTDispatchQueue.mm
* Flag set while an FSTDispatchQueue operation is currently executing. Used for assertion
* sanity-checks.
*/
@property(nonatomic, assign) BOOL operationInProgress;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I change the property to be atomic, TSan still reports a data race. I probably don't understand how Objective-C atomic works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objective-C properties are a code-generation mechanism. The attributes here manipulate the generated code for the getter and setter. In this case, the value is being accessed through the instance variable directly so the change has no effect.

If you changed the references from _operationInProgress to self.operationInProgress (and flipped the atomic attribute) that should also fix this.

@var-const var-const requested a review from wilhuff April 11, 2018 15:46
@var-const
Copy link
Contributor Author

@wilhuff Gil, a couple of notes:

  • making this variable atomic isn't necessarily the best way to synchronize, I'm open to suggestions;
  • to test from XCode, you can open Product > Scheme > Edit Scheme > Test (configuration on the left) > Diagnostics (tab) > Thread Sanitizer.

wilhuff
wilhuff reviewed Apr 11, 2018
View reviewed changes
Firestore/Source/Util/FSTDispatchQueue.mm Outdated
@@ -16,6 +16,8 @@

#import <Foundation/Foundation.h>

#import <atomic>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I just added checks to cpplint to help with that and it has called you out: https://travis-ci.org/firebase/firebase-ios-sdk/jobs/365187148#L641 :-).

Firestore/Source/Util/FSTDispatchQueue.mm Outdated
@@ -145,20 +147,20 @@ - (void)markDone {

#pragma mark - FSTDispatchQueue

@interface FSTDispatchQueue ()
@interface FSTDispatchQueue () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though customarily it's just done on the @implementation.

We use the empty category to make "private" declarations that must be in an @interface. Things like property declarations or a hidden designated initializer.

Firestore/Source/Util/FSTDispatchQueue.mm
* Flag set while an FSTDispatchQueue operation is currently executing. Used for assertion
* sanity-checks.
*/
@property(nonatomic, assign) BOOL operationInProgress;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objective-C properties are a code-generation mechanism. The attributes here manipulate the generated code for the getter and setter. In this case, the value is being accessed through the instance variable directly so the change has no effect.

If you changed the references from _operationInProgress to self.operationInProgress (and flipped the atomic attribute) that should also fix this.

wilhuff
wilhuff approved these changes Apr 11, 2018
View reviewed changes
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wilhuff
Copy link
Contributor

wilhuff commented Apr 11, 2018 •
edited
Loading

An alternative would be to change the order of this assertion:

!_operationInProgress || ![self onTargetQueue]

That is to say that we allow dispatch async from outside the queue or from within the queue if there's no operation already in progress.

@var-const
Copy link
Contributor Author

An alternative would be to change the order of this assertion

This seems reasonable. TSan is happy with the change as well. The only thing I'm not sure about is what prevents a race with _operationInProgress being set in the initializer (on the main thread, I presume), but I guess it's something to do with how Objective-C initialization works. Since TSan doesn't see an issue, it should be fine.

What's your preference? I can push the commit that does this instead of an atomic.

@wilhuff
Copy link
Contributor

wilhuff commented Apr 11, 2018

I'm not sure there is anything preventing that race, so likely we should do both (reorder the check and leave this as an atomic).

@var-const
Copy link
Contributor Author

so likely we should do both (reorder the check and leave this as an atomic).

Done. Since you approved the <atomic> version, I'll leave that (and it's slightly less verbose), but I tried atomic Objective-C property locally, and it also works (thanks for explaining).

wilhuff
wilhuff approved these changes Apr 11, 2018
View reviewed changes
Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@var-const
Copy link
Contributor Author

Thanks for the review. I'll merge once the checks complete.

@var-const var-const merged commit 9f02fa6 into master Apr 11, 2018
@var-const var-const deleted the varconst/dispatch-queue-atomic-bool branch April 11, 2018 19:18
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
@var-const
iOS: fix data race in FSTDispatchQueue. (firebase#1070)
48afcad 
operationInProgress is accessed from both the main thread and from
libdispatch on some other thread. Make it atomic to avoid a data race.
Also reorder assertion checks to only access operationInProgress
after making sure the function is running on the queue.

Tested: ran unit tests using old and new versions under Thread
Sanitizer, verified that TSan reports a data race for the old version,
but finds no issues with the new version.
@firebase firebase locked and limited conversation to collaborators Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wilhuff wilhuff wilhuff approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@var-const @wilhuff @googlebot