Skip to content
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

Removing @synchronized(self) block and call from any thread #714

Closed
wants to merge 1 commit into from

Conversation

tonyxiao
Copy link
Contributor

@tonyxiao tonyxiao commented Sep 12, 2017

Handles race between

https://github.com/segmentio/analytics-ios/blob/dev/Analytics/Classes/Integrations/SEGIntegrationsManager.m#L499 and https://github.com/segmentio/analytics-ios/blob/dev/Analytics/Classes/Integrations/SEGIntegrationsManager.m#L506 and https://github.com/segmentio/analytics-ios/blob/dev/Analytics/Classes/Integrations/SEGIntegrationsManager.m#L430.

This guarantees that forwardSelector:arguments:options: is only ever called from the _serialQueue, therefore eliminating the need to have @synchronized(self) block.

Only concern is whether certain integrations require the application state notifications hooks to be called from main thread. If so this can cause an issue.

cc @f2prateek @ladanazita

@codecov-io
Copy link

codecov-io commented Sep 12, 2017

Codecov Report

Merging #714 into dev will decrease coverage by 0.67%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev     #714      +/-   ##
==========================================
- Coverage   72.06%   71.39%   -0.68%     
==========================================
  Files          39       39              
  Lines        1629     1629              
  Branches      174      174              
==========================================
- Hits         1174     1163      -11     
- Misses        336      348      +12     
+ Partials      119      118       -1

@f2prateek
Copy link
Contributor

Only concern is whether certain integrations require the application state notifications hooks to be called from main thread. If so this can cause an issue.

I think most do require this.

Do we have another option to deliver these to the main thread, perhaps with dispatch_get_main_queue?

@tonyxiao
Copy link
Contributor Author

yes, then the issue is we won't be able to deliver to main thread without causing a potential deadlock (with the @synchronized block anyways). On a second look, maybe it's ok for this part of the code to execute concurrently because it's not actually affected by race condition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants