RUM-2600: Add traversal flag to snapshot items#1837
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #1837 +/- ##
===========================================
+ Coverage 83.49% 83.50% +0.01%
===========================================
Files 467 467
Lines 16491 16491
Branches 2483 2485 +2
===========================================
+ Hits 13769 13770 +1
+ Misses 2043 2038 -5
- Partials 679 683 +4
|
9326bc1 to
be5755d
Compare
| internal val systemInformation: SystemInformation | ||
| ) : RecordedDataQueueItem(recordedQueuedItemContext) { | ||
| internal var nodes = emptyList<Node>() | ||
| internal var isFinishedTraversal = false |
There was a problem hiding this comment.
if this one can be modified from the different threads, it should be either @Volatile or AtomicBoolean
There was a problem hiding this comment.
It's modified only on the main thread - created as false and modified to true after the traversal. It may indeed be ww to make it volatile since the value could be read on another thread
There was a problem hiding this comment.
But it is read in the isValid, isReady functions, which may be called from the worker thread:
isValidis called fromtriggerProcessingLoop, which is annotated with@WorkerThreadand fromshouldRemoveItemwhich is called fromtriggerProcessingLoop.isReadyis called also from multiple places oftriggerProcessingLoop.
Looking on the threading model and call-chain I can see that RecordedDataQueueRefs#tryToConsumeItem may call that triggerProcessingLoop and RecordedDataQueueRefs#tryToConsumeItem may be called during the traversal.
So to be on the safe side and to protect from the changes of the the threading model in the future, it is still better to add it.
There was a problem hiding this comment.
Same may be applied to the nodes property as well btw, maybe we should add @Volatile/AtomicReference there.
Anyway, passing mutable structures between the threads is already a dangerous idea.
993fb71 to
043209c
Compare
043209c to
b24509a
Compare
4 participants
What does this PR do?
Adds a flag to snapshot items that indicates that they have finished traversing the view tree. Also, increases the interval that snapshots are considered stale from 200ms to 1 second
Motivation
The RecordedDataQueueHandler in some situations could attempt to process an item that was still performing a traversal. Because items are inserted into the queue at the beginning of their traversals without nodes, the handler could regard the item as invalid and drop it from the queue. With this change, items that haven't finished traversals will only be dropped if stale.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)