Skip to content

Fixes issue where handler thread is started/stopped based on activity… #5468

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
Merged
bryanatkinson merged 2 commits into from
Oct 23, 2023
Merged

Fixes issue where handler thread is started/stopped based on activity… #5468

bryanatkinson merged 2 commits into from
Oct 23, 2023

Conversation

@bryanatkinson
Copy link
Contributor

@bryanatkinson bryanatkinson commented Oct 23, 2023

… lifecycle events. We can't have the handler thread reacting to lifecycle events because:

  • There's no guarantee that an activity is ever started on a process, but we still want session ids pushed there (eg. service-only process)
  • There can be more than one activity per process and the first time one of those activities is stopped, the handlertthread will be stopped and so we'll have no thread to process the callback messages.

Updated to start the thread on service binding and never stop it since we don't yet support unbinding.

@bryanatkinson
Fixes issue where handler thread is started/stopped based on activity…
7376415 
… lifecycle events. We can't have the handler thread reacting to lifecycle events because:

* There's no guarantee that an activity is ever started on a process, but we still want session ids pushed there (eg. service-only process)
* There can be more than one activity per process and the first time one of those activities is stopped, the handlertthread will be stopped and so we'll have no thread to process the callback messages.

Updated to start the thread on service binding and never stop it since we don't yet support unbinding.
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 23, 2023

1 Warning
⚠️ Did you forget to add a changelog entry? (Add the 'no-changelog' label to the PR to silence this warning.)

Generated by 🚫 Danger

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 23, 2023

Coverage Report 1

Affected Products

  • firebase-sessions

    Overall coverage changed from ? (9d548b5) to 51.79% (c6ca53f) by ?.

    26 individual files with coverage change

    FilenameBase (9d548b5)Merge (c6ca53f)Diff
    ApplicationInfo.kt?100.00%?
    AutoSessionEventEncoder.java?100.00%?
    Comparisons.kt?0.00%?
    Dispatchers.kt?0.00%?
    Emitters.kt?0.00%?
    EventGDTLogger.kt?75.00%?
    FirebaseSessions.kt?0.00%?
    FirebaseSessionsDependencies.kt?92.00%?
    FirebaseSessionsRegistrar.kt?0.00%?
    LocalOverrideSettings.kt?100.00%?
    RemoteSettings.kt?88.57%?
    RemoteSettingsFetcher.kt?65.85%?
    SafeCollector.common.kt?0.00%?
    SessionDatastore.kt?0.00%?
    SessionEvent.kt?100.00%?
    SessionEvents.kt?97.87%?
    SessionFirelogPublisher.kt?78.05%?
    SessionGenerator.kt?20.83%?
    SessionLifecycleClient.kt?0.00%?
    SessionLifecycleService.kt?0.00%?
    SessionsActivityLifecycleCallbacks.kt?0.00%?
    SessionsSettings.kt?59.62%?
    SessionSubscriber.kt?75.00%?
    SettingsCache.kt?94.83%?
    SettingsProvider.kt?50.00%?
    TimeProvider.kt?0.00%?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/AdvzmFxa5H.html

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2023 •
edited
Loading

Unit Test Results

  28 files  ±0    28 suites  ±0   1m 12s ⏱️ +3s
  64 tests ±0    64 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
128 runs  ±0  128 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit e81366e. ± Comparison against base commit 9d548b5.

♻️ This comment has been updated with latest results.

@bryanatkinson bryanatkinson merged commit ec89794 into sessions-nine Oct 23, 2023
@bryanatkinson bryanatkinson deleted the sessions-client-handler branch October 23, 2023 17:17
@firebase firebase locked and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mrober mrober mrober approved these changes

@jrothfeder jrothfeder Awaiting requested review from jrothfeder

Assignees

No one assigned

Labels

size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bryanatkinson @google-oss-bot @mrober