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

[$250] [Crashlytics] TaskQueue: Error with task : [Pusher] instance not found. Pusher.subscribe() most likely has been called before Pusher.init() #46119

Closed
CortneyOfstad opened this issue Jul 24, 2024 · 25 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Jul 24, 2024

Coming from this GH — #45054 (comment)

Reported by @TMisiukiewicz

Fatal Exception: com.facebook.react.common.JavascriptException: Error: TaskQueue: Error with task : [Pusher] instance not found. Pusher.subscribe()
            most likely has been called before Pusher.init(), js engine: hermes, stack:
anonymous@1:2938635
processNext@1:761178
_processUpdate@1:759885
anonymous@1:500012
_callTimer@1:498935
_callReactNativeMicrotasksPass@1:499131
callReactNativeMicrotasks@1:501081
__callReactNativeMicrotasks@1:199705
anonymous@1:197911
__guard@1:199579
flushedQueue@1:197822

       at com.facebook.react.modules.core.ExceptionsManagerModule.reportException(ExceptionsManagerModule.java:65)
       at com.facebook.jni.NativeRunnable.run(NativeRunnable.java)
       at android.os.Handler.handleCallback(Handler.java:959)
       at android.os.Handler.dispatchMessage(Handler.java:100)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29)
       at android.os.Looper.loopOnce(Looper.java:232)
       at android.os.Looper.loop(Looper.java:317)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:234)
       at java.lang.Thread.run(Thread.java:1012)
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0164ec71de125e22e3
  • Upwork Job ID: 1829520793404021988
  • Last Price Increase: 2024-08-30
Issue OwnerCurrent Issue Owner: @ZhenjaHorbach
@CortneyOfstad CortneyOfstad self-assigned this Jul 24, 2024
@CortneyOfstad CortneyOfstad added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

Current assignee @CortneyOfstad is eligible for the Bug assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Jul 29, 2024

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

@CortneyOfstad
Copy link
Contributor Author

@TMisiukiewicz it was indicated on one of the other Crashlytic GHs that the logs are truncated. Any way you could pull up the full logs? Thanks!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 31, 2024
Copy link

melvin-bot bot commented Aug 5, 2024

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

@TMisiukiewicz
Copy link
Contributor

can we change it to weekly?

Copy link

melvin-bot bot commented Aug 7, 2024

@CortneyOfstad this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Aug 7, 2024

@CortneyOfstad Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Aug 9, 2024

@CortneyOfstad 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@CortneyOfstad
Copy link
Contributor Author

Adjusting the frequency now! Sorry for the delay, as I was OoO!

@melvin-bot melvin-bot bot removed the Overdue label Aug 12, 2024
@CortneyOfstad CortneyOfstad added Weekly KSv2 and removed Daily KSv2 labels Aug 12, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 20, 2024
@CortneyOfstad
Copy link
Contributor Author

Not overdue 👍

@melvin-bot melvin-bot bot removed the Overdue label Aug 21, 2024
@OlimpiaZurek
Copy link
Contributor

Hi. I'm Olimpia from Callstack and I would like to work on this issue.

@OlimpiaZurek
Copy link
Contributor

Hey @CortneyOfstad!

Here is a proposal to fix this error:

Problem

This error indicates that the Pusher.subscribe() method is called before the Pusher.init() is successfully executed. This usually happens due to a race condition in the app. In this case, a race condition occurs because the code tries to subscribe to Pusher events before the Pusher instance is ready. Since the Pusher instance is not yet available, the subscription fails, triggering the error.

Solution

To solve this error, we need to make sure that Pusher instance initialization (Pusher.init()) is complete before any subscription calls are made. This can be done by adjusting the code to enforce the order in which Pusher.init must be completed before subscribeToUserEvents begin. This will ensure that all necessary setup steps are executed in the correct order.

At the moment subscribeToUserEvents is called in two places in AuthScreen file:

 if (!Navigation.isActiveRoute(ROUTES.SIGN_IN_MODAL)) {
             // This means sign in in RHP was successful, so we can subscribe to user events
      User.subscribeToUserEvents();
  }
 Pusher.init({
        appKey: CONFIG.PUSHER.APP_KEY,
        cluster: CONFIG.PUSHER.CLUSTER,
        authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/AuthenticatePusher?`,
    }).then(() => {
        User.subscribeToUserEvents();
    })

But only in the second case the Pusher is initialized. This can be fixed by adding a helper function to initialize the Pusher and then calling the subscribe event and invoking it in the above two places.

Git diff:

 
+function initializePusher() {
+    Pusher.init({
+        appKey: CONFIG.PUSHER.APP_KEY,
+        cluster: CONFIG.PUSHER.CLUSTER,
+        authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/AuthenticatePusher?`,
+    }).then(() => {
+        User.subscribeToUserEvents();
+    });
+}
+
 Onyx.connect({
     key: ONYXKEYS.SESSION,
     callback: (value) => {
@@ -122,7 +132,7 @@ Onyx.connect({
 
         if (Navigation.isActiveRoute(ROUTES.SIGN_IN_MODAL)) {
             // This means sign in in RHP was successful, so we can subscribe to user events
-            User.subscribeToUserEvents();
+            initializePusher();
         }
     },
 });
@@ -252,13 +262,7 @@ function AuthScreens({session, lastOpenedPublicRoomID, initialLastUpdateIDApplie
         NetworkConnection.listenForReconnect();
         NetworkConnection.onReconnect(handleNetworkReconnect);
         PusherConnectionManager.init();
-        Pusher.init({
-            appKey: CONFIG.PUSHER.APP_KEY,
-            cluster: CONFIG.PUSHER.CLUSTER,
-            authEndpoint: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/AuthenticatePusher?`,
-        }).then(() => {
-            User.subscribeToUserEvents();
-        });
+        initializePusher();

@melvin-bot melvin-bot bot added the Overdue label Aug 29, 2024
@CortneyOfstad
Copy link
Contributor Author

Looks great! Let me get some additional eyes on this as well!

@melvin-bot melvin-bot bot removed the Overdue label Aug 30, 2024
@CortneyOfstad CortneyOfstad added External Added to denote the issue can be worked on by a contributor Overdue labels Aug 30, 2024
@melvin-bot melvin-bot bot changed the title [Crashlytics] TaskQueue: Error with task : [Pusher] instance not found. Pusher.subscribe() most likely has been called before Pusher.init() [$250] [Crashlytics] TaskQueue: Error with task : [Pusher] instance not found. Pusher.subscribe() most likely has been called before Pusher.init() Aug 30, 2024
Copy link

melvin-bot bot commented Aug 30, 2024

Job added to Upwork: https://www.upwork.com/jobs/~0164ec71de125e22e3

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 30, 2024
Copy link

melvin-bot bot commented Aug 30, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ZhenjaHorbach (External)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Overdue Weekly KSv2 labels Aug 30, 2024
@CortneyOfstad CortneyOfstad removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 30, 2024
@CortneyOfstad
Copy link
Contributor Author

@ZhenjaHorbach interested to hear your thoughts on the @OlimpiaZurek's proposal here!

@ZhenjaHorbach
Copy link
Contributor

@CortneyOfstad
I will check within an hour !

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Aug 30, 2024

@OlimpiaZurek
Thanks for your proposal
I like your proposal !

I had only one concern
That we can initialize the pusher several times
But following the code this is not a problem since if we have a socket we do not do re-initialization

function init(args: Args, params?: unknown): Promise<void> {
return new Promise((resolve) => {
if (socket) {
resolve();
return;
}

So in this case the proposal looks good to me

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 30, 2024

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 2, 2024
@CortneyOfstad
Copy link
Contributor Author

PR has not yet been deployed to production, so going to continue to keep an eye on this 👍

@ZhenjaHorbach
Copy link
Contributor

Looks like automation failed here, and this PR #48664 on 09/09.
So we are ready for payment !

@ZhenjaHorbach ZhenjaHorbach mentioned this issue Sep 17, 2024
55 tasks
@ZhenjaHorbach
Copy link
Contributor

BugZero Checklist

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@ZhenjaHorbach] The PR that introduced the bug has been identified. Link to the PR:

#24083

  • [ @ZhenjaHorbach] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

#24083 (comment)

  • [@ZhenjaHorbach] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

NA

  • [@ZhenjaHorbach] Determine if we should create a regression test for this bug.
  • [@ZhenjaHorbach] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

I'm not sure that regression tests are needed here
Since this issue was quite difficult to reproduce
Plus, if something happens, we can always see it in Crashlytics

@CortneyOfstad
Copy link
Contributor Author

Sorry for the delay here @ZhenjaHorbach! I sent you an offer in Upwork here. Let me know once you accept and I can get that paid ASAP. Thanks!

@ZhenjaHorbach
Copy link
Contributor

Sorry for the delay here @ZhenjaHorbach! I sent you an offer in Upwork here. Let me know once you accept and I can get that paid ASAP. Thanks!

No worries !
And I accepted
Thanks

@CortneyOfstad
Copy link
Contributor Author

Payment Summary

@ZhenjaHorbach — paid $250 via Upwork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants