Skip to content

fix(replay): Fixes potential out-of-order segments #13609

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

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Sep 6, 2024

This fixes a potential issue where segments can come in out of order due to our flush "lock" was not being respected in two cases:

  1. No current flush in progress, but flush throws an error (this should be rare as the common errors that get thrown should stop the replay completely)
  2. Flush is in progress, which skips the code block that releases lock and then calls debouncedFlush. This leaves the lock always set to a resolved (or rejected) promise.

This ultimately should not change too much as the flush calls are debounced anyway, but this cleans up the code a bit and also logs any exceptions that may occur. However this can fix issues where segments can come in out of order depending on how long the send request takes. e.g.

image

where ideally it looks like
image

This fixes a potential issue where our flush "lock" was not being respected in two cases:

1) No current flush in progress, but flush throws an error (this should be rare as the common errors that get thrown should stop the replay completely)
2) Flush is in progress, which skips the code block that releases lock and then calls debouncedFlush. This leaves the lock always set to a resolved (or rejected) promise.

This ultimately should not change too much as the flush calls are debounced anyway, but this cleans up the code a bit and also logs any exceptions that may occur.
@billyvg billyvg marked this pull request as ready for review September 6, 2024 14:52
@billyvg billyvg requested a review from a team as a code owner September 6, 2024 14:52
@billyvg billyvg changed the title fix(replay): Fixes potential issue with too many flushes fix(replay): Fixes potential out-of-order segments Sep 6, 2024
@@ -1226,27 +1226,29 @@ export class ReplayContainer implements ReplayContainerInterface {
// TODO FN: Evaluate if we want to stop here, or remove this again?
}

// this._flushLock acts as a lock so that future calls to `_flush()`
// will be blocked until this promise resolves
const _flushInProgress = !!this._flushLock;
Copy link
Contributor

@c298lee c298lee Sep 6, 2024

Choose a reason for hiding this comment

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

what does !! do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah it's a shortcut to coerce to a boolean instead of e.g. Boolean()

@billyvg billyvg merged commit 8fcee67 into develop Sep 10, 2024
125 checks passed
@billyvg billyvg deleted the fix-replay-fix-potential-issue-with-flushing branch September 10, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants