Skip to content
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

de-async emissions #375

Closed
eoghanmurray opened this issue Sep 24, 2020 · 6 comments
Closed

de-async emissions #375

eoghanmurray opened this issue Sep 24, 2020 · 6 comments

Comments

@eoghanmurray
Copy link
Contributor

I've got a problem I've traced back to processing of the emission event.
Basically I think it's possible for a DOMMutation event to alter the DOM node tree while I'm processing the initial FullSnapshot event.

This is resulting in an application specific bug, but (assuming I've got the diagnosis right) I'm wondering whether this could be a more general source of bugs, e.g. a mutation event appearing in the recording before the snapshot that it should act upon.

Should we introduce locking code or some sort of a queue to ensure that emissions happen sequentially?

@Yuyz0112
Copy link
Member

I've seen similar problems in the checkout feature.

I've considered adding a general generation number to the events, which is the number of the full snapshot. So in the replay, we can check whether an event was happened before the current generation.

@eoghanmurray
Copy link
Contributor Author

I'm having a look at implementing a simple lock in the wrapEmit function so that the next emission can't happen until the client code deals with the previous emission.
I'm using a queue, and basing my understanding of the js details on https://stackoverflow.com/a/2734311/6691

@eoghanmurray
Copy link
Contributor Author

This is turning out a little more complicated.

I think I have to delay execution of e.g. mutation.ts::emit() itself as it is the calling of this function that modifies the mirror

(my current queue/lock system is just delaying the processing of the emissions)

Any guidance appreciated!

@eoghanmurray
Copy link
Contributor Author

Ok trying to directly pause mutations while taking a full snapshot, building on the code added here:
#224
(that pull request hasn't been accepted yet)

@eoghanmurray
Copy link
Contributor Author

I have all tests passing in #385 and believe things are correct.

I have this deployed on 11 websites that were exhibiting the problem (a FullSnapshot already containing the results of DOM Mutation before the rrweb mutation event is emitted), and it solves the problem; no need for any further work to queue emissions as far as I'm concerned.

@eoghanmurray
Copy link
Contributor Author

So to wrap up: #385 (now merged) doesn't do any general queueing of emissions (to ensure that only one event can be processed at a time) but does address the specific interaction of FullSnapshot with DomMutation ... i.e. the latter is delayed until the former is completed.

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

No branches or pull requests

2 participants