0

I have a save function in my app which can be called manually and an autosave function which runs every 60 seconds.

To prevent the two ops trying to access the same file at the same instant, I set a flag called isSaving to true when one starts running, and to false again afterward. If open or save detect that autosave is running, they wait 1000ms and try again. If they fail after that I consider it an error.

Autosave:

setInterval(autosave, 1000 * 60);
isSaving = false;

function autosave() 
{        
    return new WinJS.Promise(function (complete, error, progress) 
    {
        if(isSaving == false) // no saving op in progress
        {
            // set saving flag on
            isSaving = true;
            // write file
            return writeFile(currentFile)
                .then(function () {
                    // saving flag off
                    isSaving = false;
                    complete();
                });
        }    
        else {
            // silently abort
            complete();
        }        
    });
}

Manual save:

var saveFileAttempts = 0;
function save()
{
    return new WinJS.Promise(function (complete, error, progress)
    {
        if (isSaving == false) // no saving op in progress
        {
            // set saving flag on
            isSaving = true;
            // write file
            return writeFile(currentFile)
                .then(function () {
                    // show notification to user "file saved"
                    return showSaveNotification()
                })
                .then(function () {
                    // set saving flag off
                    isSaving = false;
                    complete();
                });
        }
        else if (saveFileAttempts < 10) { 
            // try again in 1000ms, up to 10 times
            saveFileAttempts++;
            setTimeout(function () { save(); }, 1000);
        }
        else{
            error();
        }
    });
}

Open:

var openFileAttempts = 0;
function open()
{
    return new WinJS.Promise(function (complete, error, progress)
    {
        if (isSaving == false)
        {
            return readFile()
                .then(function (file) {
                    currentFile = file;
                    openFileAttempts = 0;
                    complete();
                });
        }
        else if (openFileAttempts < 10) { 
            // try again in 1000ms, up to 10 times
            openFileAttempts++;
            setTimeout(function () { open(); }, 1000);
        }
        else{
            error();
        }
    });
}

This feels like a hack. Is there a better way to achieve what I'm trying to do?

FYI: These functions return promises because there are other functions that call them.

roryok
  • 9,325
  • 17
  • 71
  • 138
  • Start by avoiding the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572) – Bergi Sep 02 '15 at 11:04
  • @Bergi save needs to return a promise in all cases as other functions call it. If I take out the `return new WinJS.Promise` call then surely only the first `if` block will return a promise? – roryok Sep 02 '15 at 11:12
  • @roryok chain the promises, look up the questions about a promises queue, also `last`, and throttle. – Benjamin Gruenbaum Sep 02 '15 at 12:00
  • 1
    @roryok: In the first if block you don't need the `Promise` constructor as `writeFile` already returns one. In the second, there should be a lib function that returns a promise for a timeout, if there is none you can use a single `new Promise` *only* around the `setTimeout` here. In the else block, you'd just use `Promise.reject`, not the `Promise` constructor. – Bergi Sep 02 '15 at 12:30

2 Answers2

0

how about maintaining a single promise chain. Then, you might not need a setTimeout, this is a easy way, might be flawed, haven't used WinJS, writing code like it is normal promise:

setInterval(save, 1000 * 60);
var promise = Promise.resolve(); 



function save(){
    return promise.then(function(){
        return writeFile(currentFile);
    });
}

function open(){
    return promise.then(function(){
        return readFile().then(function (file) {                
            currentFile = file;
        });
    });
}

but I guess, one problem with this code is, since it is single promise chain, you need to catch error properly in your application.

mido
  • 24,198
  • 15
  • 92
  • 117
0

Instead of waiting 1000ms and trying again, I'd recommend using a promise to represent that a save is ongoing and when it will end.

var saving = null;

setInterval(function() {
    if (!saving) // ignore autosave when already triggered
        save().then(showAutoSafeNotification);
}, 60e3);

function save() {
    if (saving)
        return saving.then(save); // queue
    // else
    var written = writeFile(currentFile);
    saving = written.then(function() {
        saving = null;
    }, function() {
        saving = null;
    });
    return written;
}

You can do the same with open (and might want to abstract the written part out), although I fail to see how it interferes with an (auto)save. If you're concerned about reading the file that is already open while it is saved, I'd let the filesystem handle that and catch the error.

Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • if FileA is being saved and I open FileB before it is complete, the content of FileB, placed into the editor, could then be mistakenly saved over FileA. is `60e3` a typo? – roryok Sep 16 '15 at 11:03
  • @roryok: the assignment to `currentFile` is atomic, the two values don't clash. `60e3` is not a typo, it's scientific notation for `60000`. – Bergi Sep 16 '15 at 11:38
  • `currentFile` is only part of the story, but this answer still works for me. Thanks again. – roryok Sep 16 '15 at 11:44