Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

Avoid reporting races when open in multiple tabs #51

Closed
derat opened this issue Jan 8, 2023 · 0 comments
Closed

Avoid reporting races when open in multiple tabs #51

derat opened this issue Jan 8, 2023 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@derat
Copy link
Owner

derat commented Jan 8, 2023

The Updater class in updater.ts is susceptible to races if the web interface is open in multiple tabs. I think that the biggest problem is that it holds state in-memory while also persisting it to localStorage. When a second tab is opened, it'll load anything that the first tab has in localStorage (before rewriting the localStorage keys), and presumably both tabs will try to send it and also step on each other's toes in localStorage.

I wrote similar logging code in https://github.com/derat/ascenso/tree/main/src/log (and then updated it a bit further in a private repo). It uses tab-unique localStorage keys, and only reclaims other tabs' in-progress and queued records if they haven't been touched in a while. I think that it wouldn't be a drop-in replacement since the code in logger.ts has additional logic to resolve conflicts, e.g. when a rating or tags update for a song is received while an earlier update for the same song is queued or in-flight.

https://balpha.de/2012/03/javascript-concurrency-and-locking-the-html5-localstorage/ describes localStorage raciness and a possible workaround using Algorithm 1 from Lamport's 1985 A Fast Mutual Exclusion Algorithm paper. (I think that https://github.com/chieffancypants/fast-mutex may be another implementation of Lamport's algorithm.)

The comment from Ian Hickson on that page says that the spec describes a mutex that lets each script atomically perform multiple operations on localStorage, but Chrome (and other browsers?) apparently don't implement it. There's some discussion of the missing mutex at https://groups.google.com/a/chromium.org/g/chromium-dev/c/O7cTL4oC_VE, along with a suggestion that the Lamport algorithm doesn't work in this case. And yeah, see the warning at https://html.spec.whatwg.org/multipage/webstorage.html#introduction-15:

The localStorage getter provides access to shared state. This specification does not define the interaction with other agent clusters in a multiprocess user agent, and authors are encouraged to assume that there is no locking mechanism. A site could, for instance, try to read the value of a key, increment its value, then write it back out, using the new value as a unique identifier for the session; if the site does this twice in two different browser windows at the same time, it might end up using the same "unique" identifier for both sessions, with potentially disastrous effects.

It looks like there's a Web Locks API now that has broad support, so maybe I could use that to synchronize localStorage access across multiple tabs.

A more robust way to fix this might be to run the reporting code in a SharedWorker. It apparently isn't implemented in mobile browsers (where I don't run the web interface, but still, yuck).

The easiest thing to do might be to listen for storage events and display a warning if the site is open in two tabs.

But what I'll probably do is to change the Updater class to use instance-specific localStorage keys and only reclaim other instances' data when it hasn't been touched in a while. I don't think I need to worry much about conflicting updates across different tabs -- if I'm listening to music in multiple tabs at the same time, I have bigger problems.

@derat derat added the bug Something isn't working label Jan 8, 2023
@derat derat self-assigned this Jan 8, 2023
derat added a commit that referenced this issue Jan 9, 2023
Rename 'in-progress' to 'active', 'play reports' to 'plays',
and 'ratings and tags' to 'updates'. Non-functional change
(apart from leaving behind stale keys in localStorage).
See #51.
derat added a commit that referenced this issue Jan 9, 2023
Change the Updater class to only use localStorage to store
queued/active plays, ratings, and tags instead of
additionally storing them in instance variables. This isn't
intended to make any major changes to the class's logic, but
it'll hopefully make it easier to avoid races if multiple
tabs are open in a later change. See #51.
derat added a commit that referenced this issue Jan 9, 2023
Make the Updater class append a random ID to localStorage
keys so that multiple instances won't step on each others'
toes. See #51.

Also add code to adopt other instances' records
periodically. (This code still needs to be updated to only
do so if the other instance hasn't been active recently.)
derat added a commit that referenced this issue Jan 9, 2023
Only adopt records out of localStorage if they're more than
5 minutes old. Also add an underTest() function to avoid
this behavior when running under unit or web tests. See #51.

I think that there's still a potential race when the network
comes online and there are multiple instances open.
@derat derat closed this as completed in cfe51ca Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant