-
Notifications
You must be signed in to change notification settings - Fork 333
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
Always deliver events asynchronously to integrations. #715
Conversation
1945f2e
to
970ba3a
Compare
Codecov Report
@@ Coverage Diff @@
## dev #715 +/- ##
==========================================
- Coverage 72.06% 72.06% -0.01%
==========================================
Files 39 39
Lines 1629 1625 -4
Branches 174 173 -1
==========================================
- Hits 1174 1171 -3
Misses 336 336
+ Partials 119 118 -1 |
Btw beware of whitespace changes, they can make PR harder to review. |
@f2prateek i thought in #714 you mentioned that "certain integrations require the application state notifications hooks to be called from main thread"? In this PR we removed the ability to do synchronous delivery IN ADDITION to removing the application state notification calls from main thread. Can we confirm this won't cause issue with bundled integrations? |
Yes that's right. This will cause an issue with some integrations, I know for sure we'll need to update the Appboy integration. That's why I suggested the fix in the comments. I'm definitely open to other fixes, and it would be ideal to have a fix that allows for both (but #714 doesn't fix the issue). |
What if we simply got rid of queue / lock based synchronization for app state notifications? |
there doesn't seem to be any race conditions which would cause a problem in this particular case. So we can just call integration methods synchronously from the main thread where notifications originate. Without |
Since sometimes integrations haven't been initialized yet, we can't always do that, and still will have to deliver them async. Hence we had the |
Removes the ability for certain events to be delivered synchronously to integrations.
It would be good to do a audit of integrations which need this functionality and make the fix I suggested for those integrations.
Closes #714.