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

fix: emit false on active$ after replication is done #4136

Merged
merged 2 commits into from
Nov 28, 2022
Merged

fix: emit false on active$ after replication is done #4136

merged 2 commits into from
Nov 28, 2022

Conversation

maxnowack
Copy link
Contributor

@maxnowack maxnowack commented Nov 15, 2022

It seems that the fix for #4117 in #4124 was not complete. However, the provided test case wasn't complete either.
I've extend the test case to also test if false is emitted after replication has completed.

@pubkey
Copy link
Owner

pubkey commented Nov 24, 2022

Test looks good.
I do not have time to fix this, please investigate.

@maxnowack
Copy link
Contributor Author

maxnowack commented Nov 24, 2022

I've already tried to. It seems that these lines will never be executed.
I've tried to investigate further, but I'm to unfamiliar with the replication internals right now.
Do you have any idea which part I should look at more closely?

@pubkey
Copy link
Owner

pubkey commented Nov 25, 2022

I think that is the correct part to look at but I have no idea why these lines do not get executed.

@maxnowack
Copy link
Contributor Author

Okay, I'll try to dig into it.

@maxnowack
Copy link
Contributor Author

Maybe I have some potential fix.
I've changed the addNewTask method, so that state.events.active.down.next(false) gets called always after the task processing inside of the state.streamQueue.down promise.
I also had to move state.firstSyncDone.down.next(true) in front of it. Otherwise awaitInitialReplication would finish before active$ emits false after the first replication.
This has the implication that firstSyncDone.down now also emits true after downstreamProcessChanges is finished first.
Please look over it carefully, as I'm not really sure if that is fine or maybe has some other impact I'm now yet aware of.

@maxnowack maxnowack changed the title test: extend test case for emit active$ during replication fix: emit false on active$ after replication is done Nov 25, 2022
@pubkey pubkey merged commit 8f9d4f7 into pubkey:master Nov 28, 2022
@pubkey
Copy link
Owner

pubkey commented Nov 28, 2022

I merged this. I am not sure if it is 100% correct but I think it is an improvement compared to the current behavior and as long as the tests are green I am confident that nothing major could be broken.

@pubkey
Copy link
Owner

pubkey commented Nov 28, 2022

Added this to the changelog here: f96d264

@maxnowack maxnowack deleted the extend-replication-test branch November 29, 2022 07:01
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.

2 participants