Skip to content

RUM-1042 Fix base64 issues with multithreading#1613

Merged
jonathanmos merged 1 commit into
developfrom
jmoskovich/rum-1042/fix-multithreading
Sep 14, 2023
Merged

RUM-1042 Fix base64 issues with multithreading#1613
jonathanmos merged 1 commit into
developfrom
jmoskovich/rum-1042/fix-multithreading

Conversation

@jonathanmos

@jonathanmos jonathanmos commented Sep 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Fixes two issues that were discovered during functional tests.

The first issue is having the AsyncImageProcessingCallback held by the Base64Serializer instance, and this can lead to the next bitmap replacing the callback before the first one has completed. This can lead to the decrement of pending images happening on the wrong node. Solution: passing the asyncCallback in every call to handleBitmap.

The second issue is having the previous RumContext held by the RumContextData object. This can lead to an item being removed from the queue (lost - not processed) with prevContext A and newContext B, and the next item with prevContext B newContext B will no longer be seen as a new view - so no meta data. Solution: the RecordedDataProcessor now holds it's own prevRumContext variable and replaces it only after processing an item.

Motivation

Issues found during functional tests.

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@codecov-commenter

codecov-commenter commented Sep 12, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1613 (c22eb48) into develop (ffb98cd) will increase coverage by 0.07%.
Report is 21 commits behind head on develop.
The diff coverage is 95.56%.

@@             Coverage Diff             @@
##           develop    #1613      +/-   ##
===========================================
+ Coverage    83.55%   83.63%   +0.07%     
===========================================
  Files          452      452              
  Lines        15625    15635      +10     
  Branches      2321     2320       -1     
===========================================
+ Hits         13055    13075      +20     
+ Misses        1948     1937      -11     
- Partials       622      623       +1
Files Changed Coverage Δ
...replay/internal/processor/RecordedDataProcessor.kt 95.95% <80.00%> (+0.06%) ⬆️
...nreplay/internal/async/RecordedDataQueueHandler.kt 97.53% <100.00%> (ø)
...sionreplay/internal/async/RecordedDataQueueItem.kt 100.00% <100.00%> (ø)
...ay/internal/async/SnapshotRecordedDataQueueItem.kt 100.00% <100.00%> (ø)
.../internal/async/TouchEventRecordedDataQueueItem.kt 100.00% <100.00%> (ø)
...ay/internal/processor/RecordedQueuedItemContext.kt 100.00% <100.00%> (ø)
...replay/internal/processor/RumContextDataHandler.kt 100.00% <100.00%> (ø)
...eplay/internal/recorder/base64/Base64Serializer.kt 93.22% <100.00%> (-1.37%) ⬇️
...y/internal/recorder/base64/ImageWireframeHelper.kt 96.77% <100.00%> (+0.07%) ⬆️
...ay/internal/recorder/mapper/BaseWireframeMapper.kt 85.92% <100.00%> (+1.86%) ⬆️
... and 2 more

... and 15 files with indirect coverage changes

@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-1042/fix-multithreading branch from d36bc38 to 224ee66 Compare September 12, 2023 12:48
@jonathanmos
jonathanmos marked this pull request as ready for review September 12, 2023 13:23
@jonathanmos
jonathanmos requested a review from a team as a code owner September 12, 2023 13:23
mariusc83
mariusc83 requested changes Sep 12, 2023
View reviewed changes
Comment thread ...rc/main/kotlin/com/datadog/android/sessionreplay/internal/processor/RecordedDataProcessor.kt Outdated
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-1012/fix-rippledrawables branch from 3b0b069 to 6e89a6d Compare September 13, 2023 12:56
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-1012/fix-rippledrawables branch from 6e89a6d to ffb98cd Compare September 13, 2023 13:20
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-1042/fix-multithreading branch from 6af6ffa to d1d73a2 Compare September 13, 2023 13:29
Base automatically changed from jmoskovich/rum-1012/fix-rippledrawables to develop September 14, 2023 06:49
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-1042/fix-multithreading branch 2 times, most recently from 26b4022 to a48b1a3 Compare September 14, 2023 08:09
mariusc83
mariusc83 previously approved these changes Sep 14, 2023
View reviewed changes

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

0xnm
0xnm previously approved these changes Sep 14, 2023
View reviewed changes
Comment thread ...eplay/src/main/kotlin/com/datadog/android/sessionreplay/internal/processor/RumContextData.kt
Comment thread ...est/kotlin/com/datadog/android/sessionreplay/internal/processor/RecordedDataProcessorTest.kt Outdated
Comment thread ...est/kotlin/com/datadog/android/sessionreplay/internal/processor/RecordedDataProcessorTest.kt Outdated
@jonathanmos
jonathanmos dismissed stale reviews from 0xnm and mariusc83 via f9121d4 September 14, 2023 09:57
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-1042/fix-multithreading branch from f9121d4 to c22eb48 Compare September 14, 2023 11:52
@jonathanmos
jonathanmos merged commit 7ce76f6 into develop Sep 14, 2023
@jonathanmos
jonathanmos deleted the jmoskovich/rum-1042/fix-multithreading branch September 14, 2023 12:09
@xgouchet xgouchet added this to the 2.2.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusc83 mariusc83 mariusc83 approved these changes
@0xnm 0xnm 0xnm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jonathanmos @codecov-commenter @mariusc83 @0xnm @xgouchet