search

embrace-io / embrace-android-sdk

Embrace's Android SDK built on OpenTelemetry
https://embrace.io/docs/android/
Apache License 2.0
135 stars 11 forks source link

Only try to send failed API requests if the ApiClient is initialized #1294

Closed bidetofevil closed 2 months ago

bidetofevil commented 2 months ago

Goal

The method used to send failed requests is being invoked before the ApiService is initialized, before setSendMethod() is ever called. This leads to a crash in a background thread because a lateinit var was being accessed before it was initialized. This leads to failed_api_calls..json to re overridden, which leads to lost data.

To fix this, ditch the lateinit var and use an atomic reference to hold the queue. Before trying to access it, check its nullness, and skip if it's null. This will force the api request to retry at the next interval.

A better fix for this will be to delay the retry attempt until after ApiClient is done, but this change is a more appropriate, surgical fix, so we can use this until we make the bigger fix later.

Testing

Added unit test to verify the state of the pending API calls queue depending on whether the SendMethod is set.

github-actions[bot] commented 2 months ago

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

bidetofevil commented 2 months ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bidetofevil and the rest of your teammates on Graphite Graphite

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 75.75758% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (56a46f6) to head (9cd112e). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...nal/comms/delivery/EmbracePendingApiCallsSender.kt 75.75% 6 Missing and 2 partials :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/embrace-io/embrace-android-sdk/pull/1294/graphs/tree.svg?width=650&height=150&src=pr&token=4kNC8ceoVB&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io)](https://app.codecov.io/gh/embrace-io/embrace-android-sdk/pull/1294?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io) ```diff @@ Coverage Diff @@ ## master #1294 +/- ## ========================================== + Coverage 83.20% 83.24% +0.03% ========================================== Files 481 481 Lines 11257 11259 +2 Branches 1718 1718 ========================================== + Hits 9366 9372 +6 + Misses 1148 1145 -3 + Partials 743 742 -1 ``` | [Files with missing lines](https://app.codecov.io/gh/embrace-io/embrace-android-sdk/pull/1294?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io) | Coverage Δ | | |---|---|---| | [...nal/comms/delivery/EmbracePendingApiCallsSender.kt](https://app.codecov.io/gh/embrace-io/embrace-android-sdk/pull/1294?src=pr&el=tree&filepath=embrace-android-core%2Fsrc%2Fmain%2Fkotlin%2Fio%2Fembrace%2Fandroid%2Fembracesdk%2Finternal%2Fcomms%2Fdelivery%2FEmbracePendingApiCallsSender.kt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io#diff-ZW1icmFjZS1hbmRyb2lkLWNvcmUvc3JjL21haW4va290bGluL2lvL2VtYnJhY2UvYW5kcm9pZC9lbWJyYWNlc2RrL2ludGVybmFsL2NvbW1zL2RlbGl2ZXJ5L0VtYnJhY2VQZW5kaW5nQXBpQ2FsbHNTZW5kZXIua3Q=) | `74.44% <75.75%> (+1.71%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/embrace-io/embrace-android-sdk/pull/1294/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=embrace-io)
bidetofevil commented 2 months ago

Merge activity