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

Change initial sample collecting #1216

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Change initial sample collecting #1216

merged 1 commit into from
Mar 23, 2023

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Mar 21, 2023 •
edited
Loading

What and why?

Potential fix for crash #1213

Chain of events:

  1. Issue from @cltnschlosser mentioning the problem crashes: Crash: Could not allocate memory #1091
  2. @cltnschlosser opened a pull request fixing the issue: Remove data race caused by doing sample on rum thread #1177
  3. Tests were failing on CI, so we changed the way initial sample is taken in this commit: 24382be
  4. This change made the app crash in a different way, which was reported here: EXC_BAD_ACCESS: thunk for @escaping @callee_guaranteed @Sendable () -> () --- [__NSCFTimer fire] #1213
  5. We opened this PR to fix the issue and created a JIRA ticket to investigate threading in the rum-monitors scopes.

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
  • Run integration tests
  • Run smoke tests

@maciejburda maciejburda force-pushed the maciey/vital-info-sampler-crash branch from 434c2ac to 6cf09ca Compare March 21, 2023 11:14
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Mar 21, 2023 •
edited
Loading

Datadog Report

Branch report: maciey/vital-info-sampler-crash
Commit report: 6cf09ca

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 159 Passed, 0 Skipped, 2m 54.98s Wall Time

@maciejburda maciejburda marked this pull request as ready for review March 21, 2023 11:51
@maciejburda maciejburda requested a review from a team as a code owner March 21, 2023 11:51
maxep
maxep approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Thanks @maciejburda! So the idea

Sources/Datadog/RUM/RUMVitals/VitalInfoSampler.swift
@@ -63,20 +63,17 @@ internal final class VitalInfoSampler {
self.refreshRateReader.register(self.refreshRatePublisher)
self.maximumRefreshRate = maximumRefreshRate

// Take initial sample
RunLoop.main.perform(inModes: [.common]) { [weak self] in
Copy link
Member

Choose a reason for hiding this comment

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

Why using [.common] mode here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It fixes failing unit tests on CI. I'm looking for minimal possible code to mitigate the issue.

I'll open a ticket to properly investigate the threading within rum monitors scope and provide all the context there.

ncreated
ncreated reviewed Mar 22, 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.

Could we 🙏 add rationale to this change in PR description? I'm missing full context on how moving this code around tackles the problem. Did we manage do reproduce this crash locally? Ideally, we should add tests to ensure no more regression in this component.

@cltnschlosser
Copy link
Contributor

@ncreated Adding some context from the issue I reported.

Could we 🙏 add rationale to this change in PR description? I'm missing full context on how moving this code around tackles the problem.

#1213 has the stacktrace for the crash that points to timer.fire()
Last week I shipped without this commit and didn't have the crash. This week I shipped 1.16.0 release which does have that commit and we get get this crash

Did we manage do reproduce this crash locally? Ideally, we should add tests to ensure no more regression in this component.

I doubt it, seems to crash at a rate of about ~1/10,000.
This component has tests for the functionality.

ncreated
ncreated approved these changes Mar 23, 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.

Thanks for more context @cltnschlosser.

Let's go with this fix 👌.

@maciejburda maciejburda merged commit 9a9c9d2 into develop Mar 23, 2023
@maciejburda maciejburda deleted the maciey/vital-info-sampler-crash branch March 23, 2023 13:03
This was referenced Mar 23, 2023
Merged
Merged
@ncreated ncreated mentioned this pull request Mar 31, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuzzybinary fuzzybinary fuzzybinary approved these changes

@cltnschlosser cltnschlosser cltnschlosser approved these changes

@ncreated ncreated ncreated approved these changes

@maxep maxep maxep approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@maciejburda @cltnschlosser @fuzzybinary @ncreated @maxep