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

Race conditions with other tabs #182

Closed
Flimm opened this issue Jul 6, 2015 · 4 comments
Closed

Race conditions with other tabs #182

Flimm opened this issue Jul 6, 2015 · 4 comments

Comments

@Flimm
Copy link

Flimm commented Jul 6, 2015

Open this URL which uses Backbone.localStorage twice in two separate tabs in the same browser:

http://backbonejs.org/examples/todos/index.html

In the first tab, add a todo item labelled "a", and open the second tab, and add a todo item labelled "b". Then refresh both tabs. Both tabs will now have a list of one item, it being "b". What should have happened is that both tabs have a list of two items, "a" and "b".

This bug happens because of this piece of code in the function create:

this.records.push(model.id.toString());
this.save();

this.records may be out-of-date at this point, and the subsequent line this.save() does not consider this, and simply overwrites what other tabs may have written.

I do not know if there is an easy way to overcome this race condition, but if there isn't, there ought to be a warning in the documentation.

@mateuszwrobel
Copy link

You should do sync between tabs by yourself. Library like this should not do it for you (it just provide interface to use LocalStorage).
Storage feature provides event that you can listen to, and update your models. Example: http://html5demos.com/storage-events

@scott-w
Copy link
Collaborator

scott-w commented Feb 28, 2017

Hi @Flimm and @mateuszwrobel, thanks for contributing this.

I agree that we don't need to document this in this case - a similar issue can happen with server-side APIs so developers should generally be aware of these possibilities.

I would definitely support adding support for events firing from localStorage, however, as this could greatly enhance the usability of this library.

@scott-w scott-w closed this as completed Feb 28, 2017
@Flimm
Copy link
Author

Flimm commented Mar 1, 2017

What would it hurt to warn developers about this?

Intelligent and well-educated developers who already know about this race condition get a friendly reminder. Not so intelligent or well-educated developers like me who make bad assumptions about being able to use a library in multiple tabs get their faulty assumptions corrected.

I don't see a downside.

In general, server-side libraries in many languages frequently point out if there are thread-safe or not (although that isn't relevant in Node), so I don't think it would be at all unusual for a library's documentation to point out that the code is not "tab-safe".

@scott-w
Copy link
Collaborator

scott-w commented Mar 1, 2017

Hi @Flimm - it's common in other libraries because being thread-safe or not is a well understood statement that is usually important to know in the context of the library. People generally need to understand the effect on memory during execution of a program as the standard assumption is that it's not affected by other parts of the program.

The localStorage is more like writing to disk (and like synchronising over a network) - you don't assume that it'll remain totally in sync without doing something. If anything, I'd be more inclined to merge #174 and put efforts into improving the ability of the system to remain in sync with your local disk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants