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

Replication (remote->local) stops when HTTP codes other than 200/404 are returned #1175

Closed
ingorammer opened this issue Dec 23, 2013 · 3 comments

Comments

@ingorammer
Copy link
Contributor

I've noticed that it seems that HTTP replication (non-continuous) stops without invoking the outside caller-specified callback if a HTTP status other than 404 or 200 is returned during the replication (sync from remote->local). This could happen for example if a reverse proxy is involved which returns a 500 while it's not yet ready.

The change below (from replicate.js) should ensure that, by default, if an err is passed to the callback from .get(), and the status is not 404, and last_seq-values are not equal, then the callback is invoked with err (instead of the current behavior, which simply drops the err and never invokes the callback at all). This allows the initial code which triggered the replication to receive this error to re-start the synchronization at a later stage.

I'll try to add the relevant tests and will then submit a PR.

function fetchCheckpoint(src, target, id, callback) {
  target.get(id, function (err, targetDoc) {
    if (err && err.status === 404) {
      callback(null, 0);
    } else if (err) {
      callback(err);
    } else {
      src.get(id, function (err, sourceDoc) {
        if (err && err.status === 404 || (!err && (targetDoc.last_seq !== sourceDoc.last_seq))) {
          callback(null, 0);
        } else if (err) {
          callback(err);
        } else {
          callback(null, sourceDoc.last_seq);
        }
      });
    }
  });
}

Want to back this issue? Place a bounty on it! We accept bounties via Bountysource.

@calvinmetcalf
Copy link
Member

I like it, though why are you comparing the sequence numbers like that? It seems to me like a better idea would be to check is the status is greater then 399, an error could theoretically return the correct seq and bigcouch style stuff might correctly respond with a weird code and a different seq.

@ingorammer
Copy link
Contributor Author

I have to admit: This comparison was in the existing code. I was wondering about this myself but haven't looked into sync enough depth yet to know why the sequence numbers are compared like this ...

So instead of changing this part, I've only added code to ensure that either callback(null, ...) or callback(err) is called.

ingorammer added a commit to ingorammer/pouchdb that referenced this issue Dec 30, 2013
Replication should invoke callback if it receives HTTP error codes
other than 404 and 200. (Which might be returned from the DB server
or from an intermediary reverse proxy). It seems that there was an
oversight which could lead to HTTP 500s to otherwise trigger neither
success nor err-callback behavior.
@ingorammer
Copy link
Contributor Author

I hope that the PR is not too far from your expectations ... in terms of tests, formatting, etc. ;)

calvinmetcalf pushed a commit that referenced this issue Dec 30, 2013
Replication should invoke callback if it receives HTTP error codes
other than 404 and 200. (Which might be returned from the DB server
or from an intermediary reverse proxy). It seems that there was an
oversight which could lead to HTTP 500s to otherwise trigger neither
success nor err-callback behavior.
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