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

replicationState.active$ emits false after pending changes have synced #763

Closed
rafamel opened this issue Aug 21, 2018 · 7 comments
Closed
Labels

Comments

@rafamel
Copy link
Contributor

rafamel commented Aug 21, 2018

Case

Possible bug/FR.

Issue

As per the docs replicationState.active$ "Emits true or false depending if the replication is running. For example if you sync with a remote server and the connection dies, this is false until the connection can be reestablished."

However, it doesn't hold a true value but rather turns false as soon as changes have been replicated.

In the same way, when the connection to the remote database is lost, replicationState.error$ doesn't emit any error.

If this is intended behavior, I believe $active doesn't offer any further information to $completed, and there is no way for us to know when the connection is active or lost in a reliable manner. In that case, this would be a FR for a subscription that allowed us to - other than manually checking whether recent changes have been synchronized.

Info

  • Environment: Browser
  • Adapter: idb, memory
  • RxDB: 7.7.1
@pubkey
Copy link
Owner

pubkey commented Aug 21, 2018

From the code, the active$-state is just a mapping to the active and paused events of the pouchdb eventemitter.

        // active
        this._subs.push(
            fromEvent(evEmitter, 'active')
            .subscribe(() => this._subjects.active.next(true))
        );
        this._subs.push(
            fromEvent(evEmitter, 'paused')
            .subscribe(() => this._subjects.active.next(false))
        );

Is it intentional by pouchdb to emit paused when the replication is finished?

@rafamel
Copy link
Contributor Author

rafamel commented Aug 22, 2018

After some testing with pouch:

  • On every change, pouch will emit for:
    • active, change (pull), and paused -in that order- when the connection is active.
    • active and paused -in that order- when the connection is not active.
  • Will emit an extra pause event when it disconnects.
  • Will emit active and pause occasionally when connection is still alive and there are no changes.
  • Will emit extra pause events occasionally even when the connection is still alive.

From all this, it wouldn't be adequate to derive active$ as an indication of whether the connection is alive from the active/paused sequence. I would propose that we remain faithful to pouchdb events with subscribers. Meaning, mapping $active to active events, and adding a additional pause$ and denied$ subscriptions so we cover all event emitters.

Now, in order to know whether the connection is active/alive or not at any given time, we could add a new subscription -alive$ might be fitting- that resolves with a boolean. In order to do that, we can't simply rely on pouch events, because as per described above, there's no way to just infer it from them. However, the Sync pouch object contains state for pull and push, and pull state will only stop ("stopped") when the connection is not alive. Therefore, we could check this value whenever an active and pause event is emitted -as pause will always emit on disconnections and active will always emit after a connection is established-, so if sync.pull.state === 'stopped', we'll emit false (only if the previously emitted value was true), and otherwise we'll emit true (if the previous value was false).

What do you think?

@pubkey pubkey added the 8.0.0 label Aug 22, 2018
@pubkey
Copy link
Owner

pubkey commented Aug 22, 2018

Thank you for the investigation.
So I understood the events wrong, when I implemented the RxReplicationState 😰

I think your plan is good, lets split the observables into active$ paused$ and denied$.
Since this changes the current (broken) behavior, I added the 8.0.0-label.

@rafamel I you have some spare time, a PR to the 8.0.0-branch is welcomed.

@rafamel
Copy link
Contributor Author

rafamel commented Aug 27, 2018

Hey @pubkey , sorry for the delay, I was on a weekend trip. I'll get on it!

@pubkey
Copy link
Owner

pubkey commented Aug 29, 2018

Hi @rafamel
I tried to implement the proposed change but have some problems.
I testet the current behavior with pouchdb@7.0.0 and different to your investigation i found:

  • There is no occasional emit of paused when the connection is alive
  • There is no paused event when the remote server dies

This makes me assume that paused is exactly the opposite of active and the mapping of the 2 event-streams into the active$-state is correct.

Can you check your described behavior with pouchdb@7.0.0 and tell me if you see any difference?

@rafamel
Copy link
Contributor Author

rafamel commented Aug 30, 2018

After some testing with pouchdb@7.0.0 - here's the log:

  • On every change, pouch will emit for active, change (pull or push, depending on direction), and paused -in that order- when the connection is active.
  • Will emit active and paused -in that order- when the remote instance is not accesible (no change event).
  • Will emit an extra paused event when the remote db is no longer accessible (network disconnect).
  • Will emit an extra paused event when the connection comes back up, followed by the active, change, paused sequence if there are pending changes to be synced.
  • Doesn't seem to emit active occasionally when connection is still alive and there are no changes, but it does emit paused.

Therefore, mapping active$ to the active, paused sequence would only tell us that there has been a change, but not whether that change has been synced (we'd need change for that), or whether the connection is alive.

So since active is not emitted occasionally anymore, for the use of the original events active and paused I think it would make sense to keep the current implementation as you mentioned (while also checking if the current value is already false when a paused event fires). denied$ would also need to be added.

That leaves out two useful subscriptions to be implemented: whether there are pending changes to be pushed (active/paused but no change), and whether the connection is alive (which still can't be derived solely from the events). For this last one, I can see one problem. What I was pointing out before is that we check sync.pull.state === 'stopped' on active and paused, but that wouldn't work for replication with a push direction. Essentially, when we only have push, we'd be unable to know whether it is alive or not, unless we find another way to determine it.

pubkey added a commit that referenced this issue Sep 2, 2018
@pubkey
Copy link
Owner

pubkey commented Sep 13, 2018

I am closing this since the original issue has been solved.
A PR with alive$ is still welcomed.

@pubkey pubkey closed this as completed Sep 13, 2018
rafamel added a commit to rafamel/rxdb that referenced this issue Sep 14, 2018
rafamel added a commit to rafamel/rxdb that referenced this issue Sep 14, 2018
@rafamel rafamel mentioned this issue Sep 14, 2018
pubkey pushed a commit that referenced this issue Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants