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

roryabraham commented 2 months ago

Coming from https://github.com/Expensify/App/pull/42447#issuecomment-2126766303...

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

After opening 4 different reports, open an IOU report
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[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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 commented 2 months ago

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 https://github.com/Expensify/App/blob/432c2e708426a25969f1a2db5f718edca2931edc/src/pages/home/report/UserTypingEventListener.tsx#L52

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 https://github.com/Expensify/App/blob/432c2e708426a25969f1a2db5f718edca2931edc/src/pages/home/ReportScreen.tsx#L460-L465.

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 https://github.com/Expensify/App/blob/432c2e708426a25969f1a2db5f718edca2931edc/src/pages/home/ReportScreen.tsx#L465, 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 commented 2 months ago

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

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 2 months ago

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

roryabraham commented 2 months ago

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

jjcoffee commented 2 months ago

PR under review.

dominictb commented 2 months ago

@jjcoffee I messed up the branch a bit. Here's the new PR: https://github.com/Expensify/App/pull/43542

muttmuure commented 2 months ago

Waiting on @roryabraham's review of linked PR

roryabraham commented 2 months ago

PR merged!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/43542

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

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)

melvin-bot[bot] commented 1 month ago

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:

[ ] [@roryabraham] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@roryabraham] 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:
[ ] [@roryabraham] 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:
[ ] [@jjcoffee / @dominictb] Determine if we should create a regression test for this bug.
[ ] [@jjcoffee / @dominictb] 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.
[ ] [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

jjcoffee commented 1 month ago

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

Opening 4 different reports
Open any IOU report
Tap on the workspace name in the header
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[bot] commented 1 month ago

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 commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

mallenexpensify commented 1 month ago

@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 commented 1 month ago

@mallenexpensify Done, thanks 🙏

jjcoffee commented 1 month ago

@mallenexpensify Accepted, thanks!

melvin-bot[bot] commented 1 month ago

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

mallenexpensify commented 1 month ago

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

Expensify / App