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

[CIS-1522] Fix Memory Leaks when opening and closing channels #1812

Merged

Conversation

nuno-vieira
Copy link
Member

@nuno-vieira nuno-vieira commented Feb 23, 2022

🔗 Issue Link

CIS-1522

🎯 Goal

The main goal was to fix the memory leaks around opening and closing channels which was mostly caused by the GalleryVC, but an overall cleanup of the memory leaks on the SDK was done as well.

🛠 Implementation

Break reference cycles in multiple places of the codebase.

📝 Changes

  • Make sure to invalidate the URLSession in the URLWebSocketEngine. According to Apple, the URLSession.delegate is strongly retained and we need to make sure we call invalidateAndCancel() to clean up the URLSession.
  • Fix Ref Cycle in GalleryVC.
  • Fix multiple ref cycles in Updaters and Middlewares.
  • Correctly unsubscribe from video player observers.
  • Make sure to destroy the ChatClient in the LoginViewController so that is easier to analyse memory leaks
  • Improve the feedback from the connection banner of the DemoApp

🎨 Changes

These are the instances that are retained after playing with the application and then coming back to the login screen before the fixes and after the fixes.

Before After
image image

🧪 Testing

  • Run the DemoApp
  • Select a User
  • Open a Channel
  • Scroll and open some images and videos
  • Close a Channel
  • Repeat the last 3 steps multiple times
  • Logout
  • Open the Memory Graph and observe the live instances

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • This change follows zero ⚠️ policy (required)
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

🎉 GIF

@nuno-vieira nuno-vieira requested a review from a team as a code owner February 23, 2022 16:28
@nuno-vieira nuno-vieira added the 🐞 Bug An issue or PR related to a bug label Feb 23, 2022
@nuno-vieira nuno-vieira force-pushed the fix/CIS-1522-Memory-Leaks-When-Opening-and-closing-channels branch 3 times, most recently from 9f36b15 to 8f2fa1f Compare February 23, 2022 18:39
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #1812 (512f104) into develop (3370423) will decrease coverage by 0.12%.
The diff coverage is 72.72%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1812      +/-   ##
===========================================
- Coverage    85.50%   85.37%   -0.13%     
===========================================
  Files          234      234              
  Lines        11314    11333      +19     
===========================================
+ Hits          9674     9676       +2     
- Misses        1640     1657      +17     
Flag Coverage Δ
llc-tests 85.37% <72.72%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ocketClient/Engine/URLSessionWebSocketEngine.swift 67.60% <70.00%> (-20.86%) ⬇️
...ventMiddlewares/TypingStartCleanupMiddleware.swift 100.00% <100.00%> (ø)
Sources/StreamChat/Workers/UserListUpdater.swift 84.21% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3370423...512f104. Read the comment docs.

@nuno-vieira nuno-vieira force-pushed the fix/CIS-1522-Memory-Leaks-When-Opening-and-closing-channels branch from 42d455a to 2a590ff Compare February 23, 2022 19:10
@nuno-vieira nuno-vieira force-pushed the fix/CIS-1522-Memory-Leaks-When-Opening-and-closing-channels branch from 2a590ff to b42762a Compare February 23, 2022 19:29
@nuno-vieira nuno-vieira force-pushed the fix/CIS-1522-Memory-Leaks-When-Opening-and-closing-channels branch from da5b7fe to 26a8029 Compare February 23, 2022 19:44
@nuno-vieira nuno-vieira added the ❗️ High Priority An issue or PR that is marked as a high priority label Feb 28, 2022
@bielikb
Copy link
Contributor

bielikb commented Feb 28, 2022

@nuno-vieira codecov reports quite huge decrease in the test coverage.

@adamrushy adamrushy self-requested a review February 28, 2022 15:33
Copy link
Contributor

@adamrushy adamrushy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅

@nuno-vieira
Copy link
Member Author

nuno-vieira commented Feb 28, 2022

@nuno-vieira codecov reports quite huge decrease in the test coverage.

@bielikb The code coverage is decrasing 0.13%, it is minimal. (From 85.50% to 85.37%). This PR has a 72% coverage, of which the min is 85%. The reason is because URLSessionWebSocketEngine is not tested at all. It wasn't before, and I don't think it is worth adding tests for it, since it is just a wrapper for URLSession.

@nuno-vieira nuno-vieira merged commit 7dafc3c into develop Feb 28, 2022
@nuno-vieira nuno-vieira deleted the fix/CIS-1522-Memory-Leaks-When-Opening-and-closing-channels branch February 28, 2022 15:37
@testableapple testableapple mentioned this pull request Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug An issue or PR related to a bug ❗️ High Priority An issue or PR that is marked as a high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants