saveDatabase() error when invoked too quickly

[mromanbcn]

I am writing an app in Node that uses LokiJS.

I am manually calling saveDatabase() every time I insert an object. If I create multiple objects quickly, the saveDatabase method is invoked multiple times in a short period of time and after the first call it complains because it cannot rename the dbName~ file. The problem is that the fs.writeFile and fs.rename (lokijs.js lines 2020 - 2025) are executed asynchronously so in some cases, when the code tries to rename dbname~ to dbname, dbname~ does not exist any longer because it was already renamed in a previous callback.

I fixed the issue by making sure the temp db name is unique by changing lokijs.js line 2019 from:

var tmpdbname = dbname + '~';

to

var tmpdbname = dbname + Date.now();

The way I am calling saveDatabase is not efficient and should probably use the autoSave feature.
The reason I do it this way is because my code does not write often, so I wanted to avoid having the autosave timer function running periodically (though it may be cleaner to do so).

Would you like me to submit a pull request?

[obeliskos]

Yea loki is mostly synchronous except most persistence adapters are asynchronous (for good reason). Ideally you would pass a callback to saveDatabase and wait for it to notify you the save was completed.

I think the best way to handle this might be to async throttle database saves to occur only after you have yielded the thread (perhaps as an option?).

We could take this standalone logic :

var db = new loki('test.db');
var coll = db.addCollection('coll');

var saveQueued = false;

function throttleSave() {
  if (saveQueued) return;

  saveQueued = true;

  setTimeout(function() {
    db.saveDatabase(function(err) {
       console.log('save completed');
       saveQueued = false;
    });
  }, 1);
}

coll.insert({a:1, b:2});
// this will queue up the save for when we yield thread
throttleSave();

coll.insert({a:2:, b:3});
// this will detect a save is already queued
throttleSave();

// now that we yield thread, our timeout will fire and save (once)

So basically you call saveDatabase and keep crunching data and it wont actually save until you are done... at which point the timeout will fire and save once.

If you prefer multiple saves (not sure why but maybe use case) you could clone the LokiFsAdapter and replace all asynchronous filesystem calls with their equivalent synchronous versions. We do have a few synchronous adapters like LokiLocalStorageAdapter and LokiMemoryAdapter and those would behave as you are expecting our node fs adapter to behave.

[mromanbcn]

Thanks @obeliskos for your reply. I am including a code snippet that triggers the issue I mentioned (my real case involves multiple calls from remote clients, so even if I wait for the db.saveDatabase callback I still get errors due to timing issues as the requests are not serialized):

var loki = require('lokijs');
var db = new loki('loki.json');

var children = db.addCollection('children');
var s = 0;

function insert() {
   console.log('Inserting: ' + s);
   children.insert({name:'' + s, legs: 8});
   s++;
   db.saveDatabase();  
}

for(var i = 0; i < 100; i++) {
   setTimeout(insert, 100);
}

At the end of the run you get:
/lokijs/src/lokijs.js:2158
throw err;
^
Error: ENOENT: no such file or directory, rename 'loki.json~' -> 'loki.json'

I tried your throttleSave approach and worked. I just had to make a minor change to your code. I added another flag called saveRequested. If you call throttleSave and saveQueued is set, instead of returning, I set saveRequested to true. This way, in the db.saveDatabase callback, if I see someone tried to save while we were saving, I call throttleSave again. Otherwise, there are cases where we don't save the latest version of the Db (I have a code snippet that triggers the error in case you are interested)

I made the changes to the code and had to update some of the tests because they were failing (mostly tests that save and then load the db immediately). Unfortunately I have one test error that I am not able to resolve. Would you mind having a look?

I tried to upload my changes to a new throttlesave branch but I don't have permissions. What is your policy for submitting changes? Should I fork the project first?

Thanks again for your help.

[obeliskos]

Yes that makes sense if you kick off an async save, yield thread, and (via event?) grab the thread again to modify and save before the first save finished.

Sure go ahead and fork and I will take a look at it... we are in the process of merging changes from master to 2.0.0 so maybe once we finalize your p.r. it can be once that is complete. At that point we may want to duplicate that fix on newer 2.0.0 version.

[mromanbcn]

Exactly, that was the issue with saving but missing a later update.

Will fork an upload the code into a branch.

BTW, great work with the Db I find it very useful.

[mromanbcn]

Ok, submitted the pull request

[ohenepee]

@mromanbcn Nice elaboration in the first comment... I'm facing the same issue

[mromanbcn]

Thanks @ohenepee. I have been working with @obeliskos and we are almost done with a set of changes that will fix the issue. The pull request is almost ready

[obeliskos]

Described now in wiki here :
https://github.com/techfort/LokiJS/wiki/LokiJS-persistence-and-adapters#save-throttling-and-persistence-contention-management

[mromanbcn]

Thanks @obeliskos!

[mromanbcn]

@obeliskos, when do you plan to release a new package (1.4.3?) with the new throttle save functionality?

Thanks

[obeliskos]

techfort will do a release whenever he finds time and we bug him enough... I would think we would give people a chance to test out a week or so and I will need to update a few tests / jsdocs but we can look to do it after then

[mromanbcn]

Fair enough, I agree about the testing. I will update my package.json so I use the latest git version for now and then will use the release version when it comes out

[obeliskos]

I have updated the throttling wiki entry to cover new additions I just made.

Basically I was adding unit tests and figured I would add method to wait for queue to drain.

Then I figured I would add that logic to loadDatabase (to wait for save queue to drain) to turn this into more of a high level contention management that works within a single loki instance. Multiple loki databases saving to the same file or multiple node processes saving to the same file can still conflict with each other, but with a single loki instance this should be managed to avoid conflict.

This option is made default for upcoming 1.43 release, anyone wishing to report any issues with latest master src, feel free to report before we update npm and releases.

[dror-ventura]

I just experienced this issue, it seems like it solved my problem, Looking forward for the release.
in the mean time working with master

[obeliskos]

techfort released 1.4.3 today, so npm has this now

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.