views:

596

answers:

5

I have a function called save(), this function gathers up all the inputs on the page, and performs an AJAX call to the server to save the state of the user's work.

save() is currently called when a user clicks the save button, or performs some other action which requires us to have the most current state on the server (generate a document from the page for example).

I am adding in the ability to auto save the user's work every so often. First I would like to prevent an AutoSave and a User generated save from running at the same time. So we have the following code (I am cutting most of the code and this is not a 1:1 but should be enough to get the idea across):

var isSaving=false;
var timeoutId;
var timeoutInterval=300000;
function save(showMsg)
{
  //Don't save if we are already saving.
  if (isSaving)
  { 
     return;
  }
  isSaving=true;
  //disables the autoSave timer so if we are saving via some other method
  //we won't kick off the timer.
  disableAutoSave();

  if (showMsg) { //show a saving popup}
  params=CollectParams();
  PerformCallBack(params,endSave,endSaveError);

}
function endSave()
{  
    isSaving=false;
    //hides popup if it's visible

    //Turns auto saving back on so we save x milliseconds after the last save.
    enableAutoSave();

} 
function endSaveError()
{
   alert("Ooops");
   endSave();
}
function enableAutoSave()
{
    timeoutId=setTimeOut(function(){save(false);},timeoutInterval);
}
function disableAutoSave()
{
    cancelTimeOut(timeoutId);
}

My question is if this code is safe? Do the major browsers allow only a single thread to execute at a time?

One thought I had is it would be worse for the user to click save and get no response because we are autosaving (And I know how to modify the code to handle this). Anyone see any other issues here?

+8  A: 

JavaScript in browsers is single threaded. You will only ever be in one function at any point in time. Functions will complete before the next one is entered. You can count on this behavior, so if you are in your save() function, you will never enter it again until the current one has finished.

Where this sometimes gets confusing (and yet remains true) is when you have asynchronous server requests (or setTimeouts or setIntervals), because then it feels like your functions are being interleaved. They're not.

In your case, while two save() calls will not overlap each other, your auto-save and user save could occur back-to-back.

If you just want a save to happen at least every x seconds, you can do a setInterval on your save function and forget about it. I don't see a need for the isSaving flag.

I think your code could be simplified a lot:

var intervalTime = 300000;
var intervalId = setInterval("save('my message')", intervalTime);
function save(showMsg)
{
  if (showMsg) { //show a saving popup}
  params=CollectParams();
  PerformCallBack(params, endSave, endSaveError);

  // You could even reset your interval now that you know we just saved.
  // Of course, you'll need to know it was a successful save.
  // Doing this will prevent the user clicking save only to have another
  // save bump them in the face right away because an interval comes up.
  clearInterval(intervalId);
  intervalId = setInterval("save('my message')", intervalTime);
}

function endSave()
{
    // no need for this method
    alert("I'm done saving!");
}

function endSaveError()
{
   alert("Ooops");
   endSave();
}
Jonathon
Yep and since I have asynchronous calls in save function using the isSaving flag should let me prevent someone else from entering the function until my end save has ran...right?
JoshBerke
@Josh I updated my answer a little bit regarding the isSaving flag. All setTimeout does is schedule a function call at least some x milliseconds in the future. It doesn't mean that it will drop what it's doing and run your function -- your scheduled save won't run at the same time as another function.
Jonathon
Since save makes an asynch call to our server, the isSaving prevents a save from occuring until the EndSave function is called.
JoshBerke
@Josh That's valid reasoning since it's asynchronous. Are you worried users will slam the save button over and over? The other option is disable the save button until your callback occurs.
Jonathon
When the user initiates a save we throw up a sort of translucent panel which blocks the user from doing anything. I am more worried about the Auto Save running when the user does something which causes a save to occur
JoshBerke
@Josh Check out the end of the save function I changed. You can reset your interval when you save so users won't be bugged by an autosave right after they saved.
Jonathon
+2  A: 

All the major browsers currently single-thread javascript execution (just don't use web workers since a few browsers support this technique!), so this approach is safe.

For a bunch of references, see Is JavaScript Multithreaded?

Jeff Sternal
@Jeff +1 for links including web workers.
Jonathon
How supported are Web workers? Looks like Firefox 3.5+ haven't found a list showing browser support for this feature.
JoshBerke
I'm not entirely sure. According to John Resig (here - http://ejohn.org/blog/web-workers/), FireFox 3.5 and Safari 4 (as of July 29, 2009). He also makes the case for structuring your code so that they use web workers when supported (if I read that right). What's more, they appear to be part of the HTML 5 specification (http://www.whatwg.org/specs/web-workers/current-work/)
Jeff Sternal
+2  A: 

All major browsers only support one javascript thread (unless you use web workers) on a page.

XHR requests can be asynchronous, though. But as long as you disable the ability to save until the current request to save returns, everything should work out just fine.

My only suggestion, is to make sure you indicate to the user somehow when an autosave occurs (disable the save button, etc).

Freyday
ah, you beat me to the webworkers comment.
David Murdoch
Yep thinking about ways to indicate this to the user great point
JoshBerke
If you have a gmail account, you'll notice that their approach when autosaving a draft is to grey out the "Save Now" button and change the text to "Saving" and then to "Saved". Only when the user starts making changes to the draft does the "Save Now" button return to a clickable state.
Freyday
+2  A: 

Looks safe to me. Javascript is single threaded (unless you are using webworkers)

Its not quite on topic but this post by John Resig covers javascript threading and timers: http://ejohn.org/blog/how-javascript-timers-work/

David Murdoch
+1  A: 

I think the way you're handling it is best for your situation. By using the flag you're guaranteeing that the asynchronous calls aren't overlapping. I've had to deal with asynchronous calls to the server as well and also used some sort of flag to prevent overlap.

As others have already pointed out JavaScript is single threaded, but asynchronous calls can be tricky if you're expecting things to say the same or not happen during the round trip to the server.

One thing, though, is that I don't think you actually need to disable the auto-save. If the auto-save tries to happen when a user is saving then the save method will simply return and nothing will happen. On the other hand you're needlessly disabling and reenabling the autosave every time autosave is activated. I'd recommend changing to setInterval and then forgetting about it.

Also, I'm a stickler for minimizing global variables. I'd probably refactor your code like this:

var saveWork = (function() {
  var isSaving=false;
  var timeoutId;
  var timeoutInterval=300000;
  function endSave() {  
      isSaving=false;
      //hides popup if it's visible
  }
  function endSaveError() {
     alert("Ooops");
     endSave();
  }
  function _save(showMsg) {
    //Don't save if we are already saving.
    if (isSaving)
    { 
     return;
    }
    isSaving=true;

    if (showMsg) { //show a saving popup}
    params=CollectParams();
    PerformCallBack(params,endSave,endSaveError);
  }
  return {
    save: function(showMsg) { _save(showMsg); },
    enableAutoSave: function() {
      timeoutId=setInterval(function(){_save(false);},timeoutInterval);
    },
    disableAutoSave: function() {
      cancelTimeOut(timeoutId);
    }
  };
})();

You don't have to refactor it like that, of course, but like I said, I like to minimize globals. The important thing is that the whole thing should work without disabling and reenabling autosave every time you save.

Edit: Forgot had to create a private save function to be able to reference from enableAutoSave

Bob