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

Fix leaking EventSourceDelegate and URLSession #23

Conversation

diederich
Copy link

Requirements

  • [-] I have added test coverage for new or changed functionality
  • [+] I have followed the repository's pull request submission guidelines (I ran swift test)
  • [-] I have validated my changes against all supported platform versions

Describe the solution you've provided

I was looking for mem leaks today, and spot some coming from the EventSource here:

  • leaking URLSessions (and everything that belongs to it, like certificates, ...)
  • leaking the EventSourceDelegate class

To fix them:

  • cleanup the EventSourceDelegate from not-required usage of self (it was mixed anyways, and only having the ones required helps a bit spotting self-captures
  • fix ref-cycle self -> eventParser -> ConnectionHandler -> self
  • fix URLSession not being cleaned up properly, which kept the delegate alive

Describe alternatives you've considered

n/a

self -> eventParser -> ConnectionHandler -> self
* if it's gone right after initialization, let's not startup the
whole machinery
* if it's gone before reconnecting -> same
…egate

> Important
> The session object keeps a strong reference to the delegate until your app exits or explicitly invalidates the session. If you do not invalidate the session by calling the
> invalidateAndCancel() or finishTasksAndInvalidate() method, your app leaks memory until it exits.
@torchhound
Copy link

Hi @diederich thanks for contributing this patch! We will review and test it internally.

@torchhound torchhound requested a review from gwhelanLD May 21, 2021 18:55
@diederich
Copy link
Author

Thanks @torchhound!

@diederich
Copy link
Author

@torchhound Again, thanks for looking into this. Any updates here? (We'd love to drop our custom dependency on the branch and go back to mainline)

@RyanYuan-Iconic
Copy link

Hi team, is there any update on this issue?

@fbarbat
Copy link

fbarbat commented Oct 12, 2021

I hit this issue too. I had created this issue launchdarkly/ios-client-sdk#258 before finding this PR.

@DispoKen
Copy link

DispoKen commented Jan 4, 2022

Hello, are there any updates on this issue? We noticed that this issue may have contributed to a spike in out-of-memory crashes that we observed last week. Is there an ETA for this?

@louis-launchdarkly
Copy link

Hello all on this thread and apologize for the slow response. Thank you for your contribution and reaching out on this issue - we have merged #36 with a bit more changes on top of this change. This should address the leaking issue.

This is still an issue for the iOS client-side SDK because we haven't adopted the change there yet. We still need more time to release a new version of the iOS client-side SDK, and I will give another update to this ticket when the new version of the iOS client-side SDK is available.

@DispoKen
Copy link

Thanks for the update @louis-launchdarkly, do you have a rough estimate when there will be a new update? I'm debating if I should fork LaunchDarkly with the fixes to address the memory leaks.

@diederich diederich closed this Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants