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

graphql - await initial replication resolves on failed replication #2745

Merged
merged 2 commits into from
Dec 12, 2020

Conversation

dome4
Copy link
Contributor

@dome4 dome4 commented Dec 10, 2020

This PR contains:

  • IMPROVED TESTS
  • A BUGFIX

Describe the problem you have without this PR

I experienced that awaitInitialReplication() of the graphql replicator plugin resolves even if the replication never succeeds (see attached issue). Is this the expected behavior?

Path of issue

syncGraphQL() executes replication.run() (see code) without param and thus the default retryOnFail = true is used. If this first pull cycle fails run() is executed with the default param again (code). Works as expected.

The problem is that syncGraphQL executes run(false) on every following pull sync cycle every liveInterval ms (code). If run(false) is executed and fails this._run(retryOnFail=false) returns false, the following if check resolves and initialReplicationComplete emits true:

 if (!willRetry && this._subjects.initialReplicationComplete['_value'] === false) {
     this._subjects.initialReplicationComplete.next(true);
 }

(code)

Fix

Simply adding retryOnFail to the if statement fixes the issue. Thus, only the initial replication cycle and every retry execution of run() are able to emit true to this.subjects.initialReplicationComplete.

Discuss

The current behavior of awaitInitialReplication() is that it neither resolves nor rejects if the replication stays erroneous. I'm thinking about if it would be useful to reject the promise and cancel the whole replication after an specific timeout that can be set by the user because this timeout hardly depends on the amount of data pulled initially. Could be implemented with Promise.race() and an timeout Promise in the awaitInitialReplication() method. Looking forward to discus 👍

Todos

  • Changelog

@dome4 dome4 changed the title WIP: graphql - await initial replication resolves on failed replication graphql - await initial replication resolves on failed replication Dec 11, 2020
@pubkey
Copy link
Owner

pubkey commented Dec 12, 2020

Yes. Adding a timeout as attribute to awaitInitialReplication() makes sense.
The current behavior is intentional. RxDB is for offline first apps, and it can happen that the user opens the app and stays offline for very long. Then suddendly the user is online and awaitInitialReplication() resolves after hours.

@pubkey
Copy link
Owner

pubkey commented Dec 12, 2020

Ah wait. I misread one part.
So awaitInitialReplication() should never resolve when the replication is not completed or in a failed state.

@pubkey pubkey merged commit f68221e into pubkey:master Dec 12, 2020
@pubkey
Copy link
Owner

pubkey commented Dec 12, 2020

I merged this one. Another PR for the timeout param is welcomed.

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