Skip to content

Conversation

@visumickey
Copy link
Contributor

Fixes #7535

@google-oss-bot
Copy link

google-oss-bot commented Feb 24, 2021

Coverage Report

Affected SDKs

  • FirebasePerformance-iOS-FirebasePerformance.framework

    SDK overall coverage changed from 88.04% (0fcf354) to 88.06% (1ec9f86) by +0.03%.

    Filename Base (0fcf354) Head (1ec9f86) Diff
    FPRGaugeManager.m 94.38% 94.89% +0.51%

Test Logs

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

👍 LGTM, one question.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM with resolution of @maksymmalyhin's question

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

Thank you for the update! LGTM on CI green.

[self prepareAndDispatchGaugeData];
});
// Dispatch the already available gauge data with old sessionId.
[self prepareAndDispatchCollectedGaugeDataWithSessionId:self.currentSessionId];
Copy link
Contributor

Choose a reason for hiding this comment

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

May it happen that the new session gauge data will be associated with the old session ID? Looks like not, because addGaugeData is also protected by the serial queue. But would like to check with you just in case.

Copy link
Contributor Author

@visumickey visumickey Feb 25, 2021

Choose a reason for hiding this comment

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

That is a good question as well. I thought about as well, a lot in detail. The ideal solution I had in my mind was to make the prepareAndDispatch method to take both the gauge data and the session ID as inputs (making it more self-contained). But that lead to a lot of repetitive code in reseting the gauge data unprotected. So took this approach which has the risk you mentioned, but since both addGaugeData and prepareAndDispatch happen on the same serial queue, that issue would not happen.

Copy link
Contributor

@jeremyjiang-dev jeremyjiang-dev left a comment

Choose a reason for hiding this comment

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

LGTM! This PR has been a great learning experience for me!

@visumickey visumickey merged commit 885ddd9 into master Feb 25, 2021
@visumickey visumickey deleted the perfFixGaugeDataRaceCondition branch February 25, 2021 20:03
ziadtamim pushed a commit to ziadtamim/firebase-ios-sdk that referenced this pull request Mar 11, 2021
@firebase firebase locked and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash in FPRGaugeManager addGaugeData

5 participants