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.

Sign up for GitHub

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

Jump to bottom

RUM-1978 Fix RUM ViewController leak #1533

Merged
merged 7 commits into from
Nov 6, 2023
Merged

RUM-1978 Fix RUM ViewController leak #1533

merged 7 commits into from
Nov 6, 2023

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Oct 31, 2023 •
edited
Loading

What and why?

Removes a leak of VCs that are relying on RUMViewIdentifiable which are held in memory until they are transformed into RUMViewIdentity.

How?

By making RUMViewIdentifiable fileprivate and relying purely on RUMViewIdentity. This way VC is wrapped immediately and doesn't leak.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@maciejburda maciejburda requested a review from a team as a code owner October 31, 2023 17:58
@maciejburda maciejburda force-pushed the maciey/RUM-1978-rum-vc-leaks branch from 136bdb3 to 751c8a2 Compare October 31, 2023 17:59
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Oct 31, 2023 •
edited
Loading

Datadog Report

Branch report: maciey/RUM-1978-rum-vc-leaks
Commit report: 519bc67

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 2704 Passed, 0 Skipped, 28m 17.01s Wall Time

ncreated
ncreated previously approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Code looks good 👍, but please describe the PR accordingly to template instructions.

DatadogRUM/Sources/RUMMonitor/Scopes/Utils/RUMViewIdentity.swift Outdated Show resolved Hide resolved
DatadogRUM/Tests/Instrumentation/Views/RUMViewsHandlerTests.swift Outdated Show resolved Hide resolved
@maciejburda maciejburda merged commit f0e2081 into develop Nov 6, 2023
20 checks passed
@maciejburda maciejburda deleted the maciey/RUM-1978-rum-vc-leaks branch November 6, 2023 10:01
@maxep maxep mentioned this pull request Nov 8, 2023
Merged
8 tasks
@ncreated ncreated mentioned this pull request Dec 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ganeshnj ganeshnj ganeshnj approved these changes

@ncreated ncreated ncreated left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maciejburda @ncreated @ganeshnj