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

No Bug: Fixing DAU ping race condition on app activation. #794

Merged
merged 1 commit into from Jan 25, 2019

Conversation

@jhreis
Copy link
Contributor

jhreis commented Jan 25, 2019

I need to build out some better unit tests for this, however some larger architecture stuff will probably need to be adjusted a bit.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • My patch or PR title has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup! (or No Bug: <message> if no relevant ticket)
  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New files have MPL-2.0 license header.

Test Plan:

Screenshots:

Reviewer Checklist:

  • PR is linked to an issue via Zenhub.
  • Issues are assigned to at least one epic.
  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate test coverage exists to prevent regressions.
  • Adaquate test plan exists for QA to validate (if applicable)
@jhreis jhreis changed the title [WIP] No Bug: Fixing DAU ping race condition on app activation. No Bug: Fixing DAU ping race condition on app activation. Jan 25, 2019
@jhreis jhreis changed the base branch from development to master Jan 25, 2019
@iccub iccub self-requested a review Jan 25, 2019
@iccub
iccub approved these changes Jan 25, 2019
BraveShared/Analytics/DAU.swift Outdated Show resolved Hide resolved
BraveShared/Analytics/DAU.swift Outdated Show resolved Hide resolved
BraveShared/Analytics/DAU.swift Outdated Show resolved Hide resolved
@danishjafri88
Copy link
Contributor

danishjafri88 commented Jan 25, 2019

The change is required, let take a case:
Time fires 1st time: Calls the sendPingToServerInternal and processingPing is set as true.
before the api call is done (Should not if the time is more than api timeout but for architecture sake). The timer fires again and there is no check to make sure the api call is not done.

@jhreis
Copy link
Contributor Author

jhreis commented Jan 25, 2019

@danishjafri88 yeah, good point, will adjust.

@jhreis jhreis force-pushed the master-dau_repinging_fix branch 3 times, most recently from 4ac9f20 to f548925 Jan 25, 2019
@iccub iccub mentioned this pull request Jan 25, 2019
14 of 14 tasks complete
@jhreis jhreis force-pushed the master-dau_repinging_fix branch from f548925 to 33d1056 Jan 25, 2019
@jhreis jhreis force-pushed the master-dau_repinging_fix branch from 33d1056 to 8df7de2 Jan 25, 2019
@jhreis jhreis merged commit 7c7c408 into master Jan 25, 2019
@jhreis jhreis deleted the master-dau_repinging_fix branch Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.