[HOLD for payment 2024-07-10] Buildup of unused Pusher subscriptions

[roryabraham]

Coming from #42447 (comment)...

Bug

Opening a workspace chat via the header in an IOU report doesn't unsubscribe from the IOU report's channel. These subscriptions then build up over time, so you can end up with a ton of AuthenticatePusher calls!

Actions taken

1. After opening 4 different reports, open an IOU report
2. Tap on the workspace name in the header

Expected result

you should see a log line that says the IOU report's pusher channel has been unsubscribed

Actual result

No such log line. The pusher channel subscription is maintained unnecessarily.

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @muttmuure

[melvin-bot]

Current assignee @jjcoffee is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[dominictb]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When tapping on the workspace name in the header. No such log line. The pusher channel subscription is maintained unnecessarily.

What is the root cause of that problem?

When we tap on the workspace name in the header, we can see the [Pusher] Unbinding event with eventName of client-userIsLeavingRoom. That means the isLeavingRoom event is cleaned up properly here.

However, the client-userIsTyping event is not cleaned up well, so the channel subscription remains.

The implementation to unsubscribe from client-userIsTyping is in

App/src/pages/home/report/UserTypingEventListener.tsx

Line 52 in 432c2e7

Report.unsubscribeFromReportChannel(reportID);

We can see that it only unsubscribe if the screen becomes unfocused. We're not unsubscribing when the screen unmounts, like we did with the client-userIsLeavingRoom in there.

So in case the screen unmounted completely, the client-userIsTyping is not unsubscribed.

What changes do you think we should make in order to solve the problem?

Unsubscribe the client-userIsTyping when the screen is unmounted.

We have the unsubscription logic here, we can put it in the clean up function of an useEffect

useEffect(() => {
        return () => {
            if (didSubscribeToReportTypingEvents.current) {
                didSubscribeToReportTypingEvents.current = false;
                InteractionManager.runAfterInteractions(() => {
                    Report.unsubscribeFromReportChannel(reportID);
                });
            }
        }
    }, []);

In here we don't need to check for topmostReportId because in case of unmounting, of course the current report will no longer be shown to the user.

Just to note this is the same pattern we used for unsubscribing from the userIsLeaving event

App/src/pages/home/ReportScreen.tsx

Lines 460 to 465 in 432c2e7

	interactionTask.cancel();
	if (!didSubscribeToReportLeavingEvents.current) {
	return;
	}
	Report.unsubscribeFromLeavingRoomReportChannel(report.reportID);

.

So after both events are unsubscribed correctly, the channel itself will be unsubscribed.

The logic to unsubscribe userIsTyping is both used for unfocused and unmounted case, so it can be put in a method for DRY.

What alternative solutions did you explore? (Optional)

In

App/src/pages/home/ReportScreen.tsx

Line 465 in 432c2e7

Report.unsubscribeFromLeavingRoomReportChannel(report.reportID);

, unsubscribe both events, or unsubscribe the entire channel by calling unsubscribe with empty event name.

Also to note, we're subscribing and unsubscribing the userIsTyping and userIsLeaving in 2 different places, it's possible that we can combine them in 1 place, either the place of userIsTyping now or of the userIsTyping.

[jjcoffee]

@dominictb's proposal LGTM! I like the idea of having both events unsubscribe in the same useEffect.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @roryabraham is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[roryabraham]

Thanks for the proposal @dominictb and my apologies for the delay in reviewing it. LGTM 👍🏼

[jjcoffee]

PR under review.

[dominictb]

@jjcoffee I messed up the branch a bit. Here's the new PR: #43542

[muttmuure]

Waiting on @roryabraham's review of linked PR

[roryabraham]

PR merged!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: unsub userIsTyping event on report screen unmount #43542

If no regressions arise, payment will be issued on 2024-07-10. 🎊

For reference, here are some details about the assignees on this issue:

• @jjcoffee requires payment (Needs manual offer from BZ)
• @dominictb requires payment (Needs manual offer from BZ)

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: N/A - existing behaviour
• 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: N/A
• 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: N/A
• Determine if we should create a regression test for this bug. Yes
• 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.

Regression Test Proposal

1. Opening 4 different reports
2. Open any IOU report
3. Tap on the workspace name in the header
4. Verify that you see a log line like [Pusher] After unbinding ${eventName} from channel ${channelName}, no other events were bound to that channel. Unsubscribing...

Note: In order to see the full logs in Chrome, you need to open DevTools and then tap the down arrow next to "Default levels" and switch to "Verbose".

Do we agree 👍 or 👎

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @jjcoffee paid $(AMOUNT) via Upwork (LINK)
• ROLE: @dominictb paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@muttmuure)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[roryabraham]

Thanks @jjcoffee. @muttmuure this is done - all that's left is to add the regression tests and pay this out

[melvin-bot]

@jjcoffee, @muttmuure, @roryabraham, @dominictb Eep! 4 days overdue now. Issues have feelings too...

[mallenexpensify]

@dominictb @jjcoffee , can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01f0dc2c395e40f075

Thanks for the Regression Test steps @jjcoffee

• https://github.com/Expensify/Expensify/issues/413187

[dominictb]

@mallenexpensify Done, thanks 🙏

[jjcoffee]

@mallenexpensify Accepted, thanks!

[melvin-bot]

@jjcoffee, @muttmuure, @roryabraham, @dominictb 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[mallenexpensify]

Contributor: @dominictb paid $250 via Upwork
Contributor+: @jjcoffee paid $250 via Upwork