couchbase / couchbase-lite-ios

Lightweight, embedded, syncable NoSQL database engine for iOS and MacOS apps.
Apache License 2.0
1.62k stars 297 forks source link

_changes does not honour timeout parameter #426

Closed refractalize closed 9 years ago

refractalize commented 10 years ago

_changes doesn't seem to honour the timeout query string parameter.

This is a problem in longpoll connections when the network infrastructure (browser, firewall) closes the connection before a change comes through. We get half a JSON response instead of a full one.

snej commented 10 years ago

Can you describe how you're using _changes? Are you talking about Couchbase Lite's REST API?

refractalize commented 10 years ago

Sorry, yes through the REST API, like this:

http://lite.couchbase./db/_changes?timeout=25000&style=all_docs&include_docs=true&feed=longpoll&since=4&limit=100&_nonce=XbkflAnoKT1pfYaC

The timeout is 25 seconds, but the browser closes the request after 30 seconds and breaks the response.

snej commented 10 years ago

I'm still confused :) You talk about network infrastructure closing the connection, but the URL you show is the internal one (lite.couchbase.) which doesn't go through any kind of network stack — it's routed directly through a custom URLProtocol within the app.

Also, I've found that if you are parsing long-poll changes feeds you must be prepared to handle "half a JSON response" because the connection could get closed for arbitrary reasons by intermediaries (routers, firewalls, gateways) before a change is sent.

Using a short timeout to work around this is inefficient, because it means you're going to be sending changes requests every 25 seconds.

refractalize commented 10 years ago

I'm doing AJAX requests in the UIWebView, using PouchDB.

I thought, possibly incorrectly, that the timeout was there to beat the infrastructure from timing out and breaking the response. That's how I've been using it in CouchDB, and PouchDB seems to be using it for the same reason.

Using a short timeout to work around this is inefficient, because it means you're going to be sending changes requests every 25 seconds.

I'm going to have to do that every 29 seconds anyway :)

Anyway, I'm happy to write a patch if you're happy to accept it. All advice welcome.

snej commented 10 years ago

Something weird is going on here — this code's been around for quite a while, and there are a number of UIWebView/JS-based apps using it, and yet this issue hasn't come up.

Paging @jchris — Is there some best practice for accessing the CBL changes feed from a PhoneGap-type app that isn't being followed here? Any idea why we're hitting this issue now and not back in 2012 or so?

snej commented 10 years ago

BTW, if we do need to improve the changes-feed generation, we should add heartbeat before timeout, as it's more efficient. (Sending a CRLF periodically will keep most intermediaries from closing the connection.)

snej commented 9 years ago

I still don't understand the initial problem statement. Regardless of whether or not it's useful to implement timeout or heartbeat, neither of them should be necessary for a local/embedded app.

I'm doing AJAX requests in the UIWebView, using PouchDB.

This actually confuses me even further. You're running both PouchDB and Couchbase Lite in the same app? Why would you do that when they basically do the same thing?

refractalize commented 9 years ago

Hi @snej, I'm using PouchDB as a client, not as a DB. Sorry, should have mentioned that. The PouchDB client is the one that sets the timeout.

jchris commented 9 years ago

The only reason to have a timeout parameter is if you need to the server to enforce a timeout. This can be useful if the client doesn't have access to the low level connection (or the ability to close it early).

In general, I've never used the timeout parameter with anything that creates a changes feed. I just catch connection closed errors and retry the request.

I also don't think there's ever a reason to try to parse a partial response.

The only edge case I can think of is if there is a changes response with so many rows that it can't be transmitted in one go, either due to intermediary timeouts, or maybe the client device doesn't have enough memory to handle it all at once, etc. In which case it makes sense to throw a limit=1000 or something onto your longpoll changes request, to force pulling down and processing a few rows at a time.

Does this help?

refractalize commented 9 years ago

I have used timeout before successfully when making HTTP requests with regular CouchDB, to avoid this issue. PouchDB uses the same strategy, evidently, and forcing a change in PouchDB just to support couchbase-lite in this particular scenario didn't seem right.

@jchris I totally understand your reasoning, and if I was writing the API raw I'd have done it that way already. But I'm stuck between two different open source projects, and chose the one that I thought made more sense to make the change.

Honestly however I'm not currently affected by this bug so don't keep it open for my benefit. Of course it's useful documentation to anybody else who might come across it. You may have an interest in maintaining parity with the CouchDB API, but I'll leave that to your judgement.

snej commented 9 years ago

I have used timeout before successfully when making HTTP requests with regular CouchDB, to avoid this issue

Isn't the only difference that you'll get a response that's complete JSON with no docs in it, instead of one that's truncated JSON? In either case you'd have to respond by repeating the call. I kind of prefer the truncated response, actually, because it tells you that the network terminated the request, not the server itself.

The drawback of specifying the timeout yourself is that you're trying to second-guess the network and the server. If you say timeout=30000 then you will end up sending requests every 30 seconds no matter what, whereas if you leave out the timeout you may get a lot more time before the request times out, giving you less work to do and less bandwidth used.

Anyway, at an implementation level this issue has morphed into a task of adding a heartbeat parameter, which is generally more useful. I just wanted to make sure I correctly understood the situation that caused you to file the issue, in case we were missing something else that needed to be done. Thanks!

pasin commented 9 years ago

We have implemented and merged the heartbeat feature to the master. Closing the ticket now.