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

SQLitePlugin Fails after 1000 transactions on iOS #190

Closed
n8sabes opened this issue Feb 23, 2015 · 35 comments
Closed

SQLitePlugin Fails after 1000 transactions on iOS #190

n8sabes opened this issue Feb 23, 2015 · 35 comments

Comments

@n8sabes
Copy link

n8sabes commented Feb 23, 2015

Execute 1000 "transactions" sequentially and the app freezes up preventing further Sqlite calls. Completion functions are no longer called after 500~1000 cycles causing the freeze. Here is an example of the problem:

https://github.com/n8sabes/ionic-sqlite-1000-transactions-ios-bug

Any input on the cause / fix would be appreciated.

@brodycj
Copy link
Contributor

brodycj commented Feb 23, 2015

I will try this case sometime later today and let you know.

@n8sabes
Copy link
Author

n8sabes commented Feb 23, 2015

Thank you -- much appreciated.

@brodycj
Copy link
Contributor

brodycj commented Feb 23, 2015

Unfortunately $cordovaSQLite.execute is a making separate remote call for each sql statement. For so many calls, this could cause problems in the internal Cordova plugin queuing, and also the internal threadpool has a limited size. I recommend you consider using $cordovaSQLite.insertCollection to insert the items.

@brodycj
Copy link
Contributor

brodycj commented Feb 23, 2015

I also see $cordovaSQLite.nestedExecute which I do not understand and looks a bit ugly. Also, you can do the transaction on the db object directly, i.e.

db.transaction(function(tx) {
  tx.executeSql(...)
  ...
});

as described in the readme.

@n8sabes
Copy link
Author

n8sabes commented Feb 23, 2015

The plugin will still fail after a total number "uses" (500-1000 "transactions") during the lifecycle of the app. Transactions can be empty, use executeSql, collections, or whatever and the failure still occurs.

Try reducing the loop count to 100, then click the insert button 5-10 times which simulates real world use and the iOS SQLite failure occurs after the same cycle count you saw in the concentrated loop test.

@n8sabes
Copy link
Author

n8sabes commented Feb 23, 2015

I create two additional tests (using davibe/Phonegap-SQLitePlugin and WebSQL) and believe I am seeing the same issue as with your plugin. It would be great if we could get a second opinion on this, possibly from Davide or someone else with a vested interest in iOS local storage.

davibe/Phonegap-SQLitePlugin#33
If I retrofitted the failure example correctly with Davide's code, it appears have a similar failure (after >1500 transactions).

Unfortunately, there does not appear to be a workaround calling directly, because the issue seems to be with the the transaction wrapper callback cycle count.

Am I missing something, or will all iOS apps using SQLite or WebSQL fail after 500-1500 database uses (transactions)?

@brodycj
Copy link
Contributor

brodycj commented Feb 23, 2015

#186 could be related. I will look at this later tonight.

@n8sabes
Copy link
Author

n8sabes commented Feb 24, 2015

*** NEW INFO ***
If you switch between apps, or exit and re-open the app (CMD+SHIFT+H / Home Button), the outstanding callbacks get called. WebSQL completes all outstanding calls and SQLite only completes 1-2 per app switch.

I've re-verified the same bug occurs regardless of the plugin targeting WebSQL or SQLite. The cycle count before going deaf is different, however. Cordova-SQLitePlugin goes deaf after 506 cycles when targeting SQLite and 1301 cycles when targeting WebSQL. This may vary depending upon your environment.

The example project has been updated for testing both SQLite and WebSQL.
https://github.com/n8sabes/ionic-sqlite-1000-transactions-ios-bug

@brodycj
Copy link
Contributor

brodycj commented Feb 24, 2015

If we can reproduce the bug on WebSQL as well, it appears to have a source in either Cordova or maybe even the iOS framework itself. I will work on this later today.

@brodycj
Copy link
Contributor

brodycj commented Feb 24, 2015

The next thing I want to do is to reproduce the issue outside of the Ionic framework. If you have a chance to make the test program outside of Ionic it would help save me some time. Thanks for your efforts so far.

@n8sabes
Copy link
Author

n8sabes commented Feb 24, 2015

Built a project w/o ionic:
https://github.com/n8sabes/deafsql.git

WebSQL executes and fails (goes deaf) after 998 cycles. Switching apps makes it complete.

Not sure why the SQLite test doesn't work on the iOS simulator after opening the DB. Seems to fail on the create table query.

Hope this helps.

@brodycj
Copy link
Contributor

brodycj commented Feb 24, 2015

We see the problems with the sqlite plugin test (on the iOS simulator) due to a simple mistake to keep the opened db variable local. This fix does the trick (fixed in both project www and platforms/ios/www):

$ git diff
diff --git a/platforms/ios/www/js/index.js b/platforms/ios/www/js/index.js
index 26195b1..575191b 100644
--- a/platforms/ios/www/js/index.js
+++ b/platforms/ios/www/js/index.js
@@ -34,7 +34,7 @@ var app = {

     SQLiteTest: function () {
         console.log('*** SQLite Test Starting')
-        var db = window.sqlitePlugin.openDatabase({name: "my.db"});
+        db = window.sqlitePlugin.openDatabase({name: "my.db"});
         console.log('SQLite DB: ' + db);
         this.initDB();
     },
diff --git a/www/js/index.js b/www/js/index.js
index 26195b1..575191b 100644
--- a/www/js/index.js
+++ b/www/js/index.js
@@ -34,7 +34,7 @@ var app = {

     SQLiteTest: function () {
         console.log('*** SQLite Test Starting')
-        var db = window.sqlitePlugin.openDatabase({name: "my.db"});
+        db = window.sqlitePlugin.openDatabase({name: "my.db"});
         console.log('SQLite DB: ' + db);
         this.initDB();
     },

@brodycj
Copy link
Contributor

brodycj commented Feb 24, 2015

I think I have now found the source in the plugin Javascript. There was a change last summer to use window.setTimeout(fun, 0); in the nextTick() function,m to start each transaction in the next "tick" in the HTML/Javascript event processing queue. This is a nice idea but causes the failure when there are too many transactions being started and/or committed at the same time. The following changes to SQLitePlugin.js seem to work around the issue:

@@ -68,7 +69,8 @@ cordova.define("com.brodysoft.sqlitePlugin.SQLitePlugin", function(require, expo
       };
     }
     txLocks[this.dbname].queue.push(t);
-    this.startNextTransaction();
+    //this.startNextTransaction();
+    if (txLocks[this.dbname].queue.length === 1) this.startNextTransaction();
   };

   SQLitePlugin.prototype.transaction = function(fn, error, success) {
@@ -90,14 +92,17 @@ cordova.define("com.brodysoft.sqlitePlugin.SQLitePlugin", function(require, expo
   SQLitePlugin.prototype.startNextTransaction = function() {
     var self;
     self = this;
-    nextTick(function() {
+    //console.log("startNextTransaction in next tick");
+    //nextTick(function() {
+    //  console.log("next tick:");
       var txLock;
       txLock = txLocks[self.dbname];
+      console.log("startNextTransaction txLock.queue.length " + txLock.queue.length + " txLock.inProgress " + txLock.inProgress);
       if (txLock.queue.length > 0 && !txLock.inProgress) {
         txLock.inProgress = true;
         txLock.queue.shift().start();
       }
-    });
+    //});
   };

   SQLitePlugin.prototype.open = function(success, error) {
@@ -192,6 +197,7 @@ cordova.define("com.brodysoft.sqlitePlugin.SQLitePlugin", function(require, expo

My next step is to rework the mechanism to start only one transaction at a time in the database object, in the next tick.

@n8sabes
Copy link
Author

n8sabes commented Feb 25, 2015

Fantastic!!!

@brodycj
Copy link
Contributor

brodycj commented Mar 3, 2015

I just found a much less drastic solution:

--- a/platforms/ios/www/plugins/com.brodysoft.sqlitePlugin/www/SQLitePlugin.js
+++ b/platforms/ios/www/plugins/com.brodysoft.sqlitePlugin/www/SQLitePlugin.js
@@ -260,6 +260,7 @@ cordova.define("com.brodysoft.sqlitePlugin.SQLitePlugin", function(require, expo
     this.executes = [];
     tx = this;
     handlerFor = function(index, didSucceed) {
+      txLocks[tx.db.dbname].inProgress = false;
       return function(response) {
         var err;
         try {

I will integrate (port) this back to the CoffeeScript and make a release soon.

@nolanlawson
Copy link
Contributor

Ah sorry, looks like that was my bad: db9296c7.

On the other hand, it looks like I did it for a good reason. This test will fail if you don't have the setTimeout. The test ensures compliance with the WebSQL spec, which stipulates that the transaction call shouldn't block the main thread but instead defer to the event loop.

If the issue is that the event loop starts to get hopelessly blocked up, I believe we could work around it by using a setImmediate shim instead of setTimeout: https://github.com/YuzuJS/setImmediate

@brodycj
Copy link
Contributor

brodycj commented Mar 3, 2015

On Tue, Mar 3, 2015 at 2:10 PM, Nolan Lawson [email protected]
wrote:

Ah sorry, looks like that was my bad: db9296c
db9296c7.

On the other hand, it looks like I did it for a good reason. This test
https://github.com/brodysoft/Cordova-SQLitePlugin/blob/db9296c724a9d41bdcdab622f315359a2dbba4db/test-www/www/index.html#L427-L442
will fail if you don't have the setTimeout.

Yes, agreed, and it fixed the PouchDB integration. What we didn't
anticipate (and I need to start reviewing pulls more carefully) is what
happens if the plugin is bombarded with transaction requests in a loop like
this. It took a bit of trial-and-error experimentation until I thought to
try clearing the inProgress flag in the internal handleFor callback
function.

If the issue is that the event loop starts to get hopelessly blocked, I
believe we could work around it by using a setImmediate shim instead of
setTimeout: https://github.com/YuzuJS/setImmediate

This is really cool idea, though this does not seem (yet) to be a real
problem and I want to avoid embedded dependencies as much as possible.

To do this right, we should remove the code to clear the inProgress flag
from some other places (and REALLY test it!) Maybe sometime later.

@nolanlawson
Copy link
Contributor

I am 99% sure we don't rely on that behavior in PouchDB (but I should check 😃). I was just trying to get the plugin to confirm more to the spec.

@n8sabes
Copy link
Author

n8sabes commented Mar 3, 2015

It's not just getting hopelessly blocked from a loop, as I recall. This bug occurs after the total number of transactions, all of which do not need to be in a single loop. They could occur over time -- minutes, hours, days -- even if the app goes into the background. So long as the app doesn't terminate and relaunch, the issue occurs. Just don't want to lose sight of this point.

@brodycj
Copy link
Contributor

brodycj commented Mar 3, 2015

@nolanlawson thanks again for your work last summer to help improve the conformance to the spec. I want to avoid breaking the conformance.

Thanks @n8sabes for the extra clarification.

Unfortunately the second solution I posted above is also causing test failures.

@nolanlawson
Copy link
Contributor

@brodybits Another option: couldn't you do something like:

var someGlobalArray = [];
var timeout = null;

/* ... */

someGlobalArray.push(transaction);

if (timeout) {
  clearTimeout(timeout);
}

timeout = setTimeout(function () {
  someGlobalArray.forEach(function (transaction) {
    // process the transaction
  });
}, 0)

Then you'd guarantee that only one function gets executed after control is returned to the event loop, and all the transactions are processed at once. Fits the spec, and doesn't overload the event loop.

@brodycj
Copy link
Contributor

brodycj commented Mar 4, 2015

I tried the following change to reduce the extra "timers":

@@ -36,14 +36,15 @@ cordova.define("com.brodysoft.sqlitePlugin.SQLitePlugin", function(require, expo

   SQLitePlugin = function(openargs, openSuccess, openError) {
     var dbname;
     console.log("SQLitePlugin openargs: " + (JSON.stringify(openargs)));
     if (!(openargs && openargs['name'])) {
       throw new Error("Cannot create a SQLitePlugin instance without a db name");
     }
+    this.hasNextTick = false;
     dbname = openargs.name;
     this.openargs = openargs;
     this.dbname = dbname;
     this.openSuccess = openSuccess;
     this.openError = openError;
     this.openSuccess || (this.openSuccess = function() {
       console.log("DB opened: " + dbname);
@@ -86,15 +87,18 @@ cordova.define("com.brodysoft.sqlitePlugin.SQLitePlugin", function(require, expo
     }
     this.addTransaction(new SQLitePluginTransaction(this, fn, error, success, true, true));
   };

   SQLitePlugin.prototype.startNextTransaction = function() {
     var self;
     self = this;
+    if (self.hasNextTick) return;
+    self.hasNextTick = true;
     nextTick(function() {
+      self.hasNextTick = false;
       var txLock;
       txLock = txLocks[self.dbname];
       if (txLock.queue.length > 0 && !txLock.inProgress) {
         txLock.inProgress = true;
         txLock.queue.shift().start();
       }
     });

I came to realize that the use of Q promises creates an extra "timer" per deffered promise. This becomes significant when there is a flood of responses, as we see in this scenario. In fact, I am thinking that this is why we have problems with the Web SQL version of the test.

@Jeff86
Copy link

Jeff86 commented Mar 5, 2015

I'm in front of the same problem, need to init one table with more than 30k rows. I'm using your plugin in ionic (angularjs) and I saw that you said we should use $cordovaSQLite.insertCollection to insert the items.

I don't find any example nor code for this "insertCollection" function, could you please help me understand how to use it?

Thanks for your help!

@erikvorkink
Copy link

Hopefully some good news: switching the way Cordova communicates to iOS from using iframes to XHR fixed this issue for us.
To do so, we added the following to our JavaScript, after deviceready:
cordova.exec.setJsToNativeBridgeMode(cordova.exec.jsToNativeModes.XHR_OPTIONAL_PAYLOAD);

After much debugging on both the iOS and JS sides, we realized that at some point Cordova seemed to stop sending messages to the iOS side. Items -were- being added to commandQueue (within cordova.js) but the iOS side wasn't receiving them. Our debugging suggested that pokeNativeViaIframe() was being called as expected, but after a while didn't have any effect. (In our case that was reliably after approx. 1,000 database transactions, although other types of communications to iOS might have the same end result.)

Rather than dive too deep into how the iframes approach worked, we switched to XHR and our problem vanished. This post suggests that the iOS default was changed from XHR to iframe due to a bug: https://github.com/apache/cordova-ios/blob/master/guides/Changing%20the%20JavaScript%20to%20Native%20Bridge%20Mode.md
So far we haven't seen any negative consequences of using XHR in our app.

Our theory is that the SQLite plugin is bumping into a Cordova bug. Or maybe it's a combination of bugs. In any case, our app is working and that's a relief. Now to figure out how to bill for 3 days of bug chasing... :-)

@Jeff86
Copy link

Jeff86 commented Mar 6, 2015

@erikvorkink Your solution works, thanks! Indeed the other "little" problem now is that my 50mb memory app skyrock to 1.2gb and takes 3min to init db... but that's another problem ;)

@MetaMemoryT
Copy link

@Jeff86 a pre-populated database can improve user experience. Also, an app can download a pre-populated database from torrent or server, if it cannot be bundled with a specific release.

@brodycj
Copy link
Contributor

brodycj commented Mar 16, 2015

Thanks @erikvorkink. Will document this when I get a chance.

@sebi-dev
Copy link

sebi-dev commented Apr 9, 2015

I added the following to my JavaScript (app.js in device ready function before DB init):
cordova.exec.setJsToNativeBridgeMode(cordova.exec.jsToNativeModes.XHR_OPTIONAL_PAYLOAD);
and it's working but it causes errors in console like : ([Error] Failed to load resource: The requested URL was not found on this server. (!gap_exec, line 0)). I think for every DB insert I get an error like this.

I have also read a post that say : changing the bridge to XHR will make the app 20% slower than iframe bridge

So what is the best way to solve this issue ? Change the bridge (to XHR) and ignore all that console errors or is better to do some changes in SQLite.js (probably to nextTick(function() )

@sebi-dev
Copy link

just update cordova IOS to 3.8.0 and everything will be fine . (https://cordova.apache.org/announcements/2015/02/25/cordova-ios-3.8.0.html)

They say that: CB-8002 (CB-7735) Update cordova.js to include bridge fix. I have test it and it's working fine

@brodycj
Copy link
Contributor

brodycj commented Apr 10, 2015

@sebi-dev happy to hear!

@sebi-dev
Copy link

happy to get everything work :)

@Heshyo
Copy link

Heshyo commented Jul 3, 2015

@sebi-dev My app would also just stop displaying things, as if it couldn't retrieve things from Pouch. I updated from 3.7.0 to 3.8.0 and it looks like I don't have the issue anymore.

@sebi-dev
Copy link

sebi-dev commented Jul 3, 2015

great !

@kievsash
Copy link

About changing NativeBridgeMode:

As I see now cordova.exec object doesn't have setJsToNativeBridgeMode method (and jsToNativeModes collectio as well) (checked it on deviceready event). Correct me if I'm wrong. Did cordova deprecate this method?
Wanted to try it out cause our cordova application experience our of memory issue while working with sqlite on IPad devices with 1Gb of memory (IPad mini). Ipad Pro (4Gb of Ram) is doing well by the way

@brodycj
Copy link
Contributor

brodycj commented Mar 23, 2017

Did cordova deprecate this method?

I would not be surprised. I think they did update the cordova-ios JavaScript-native bridge within the past 1-2 years and would not expect this to be an issue anymore.

In general this kind of issue will be supported in litehelpers / Cordova-sqlite-evcore-extbuild-free which available under GPL or commercial license options. There is a commercial license giveaway with 8 days left, please see storesafe/cordova-sqlite-evcore-extbuild-free#17 for details.

rafaellop added a commit to rafaellop/cordova-sqlite-storage that referenced this issue Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants