search

arj03 / ssb-browser-demo

A secure scuttlebutt client interface running in the browser
Other
58 stars 11 forks source link

Basic protection against concurrency issues #259

Closed KyleMaas closed 3 years ago

KyleMaas commented 3 years ago

Basic protection against concurrency issues. See #139. Uses a localStorage-based mutex which times out in case a tab dies on us. Adds a bunch of extra checks throughout the codebase to try to gracefully handle a locked SSB.

Also fixes:

arj03 commented 3 years ago

This is really exciting! I'll be in a call with mix soon so don't have time to properly review this, and it needs because it's a quite big change. Is it ok if I create a branch with some changes while I review it? Have some ideas to make this a bit simpler maybe. Also if you create your branches in this repo instead of KyleMass, then it should be easier to have that kind of workflow.

KyleMaas commented 3 years ago

Go for it. This should allow you to make changes, though - I left that on.

If you do make changes, there are a few important things about this:

  1. It is possible for a not-quite-fully-initialized SSB to be passed back to the caller. This pretty much always happens with the first getSSB() call that initializes the SSB object. This is why in most places I don't just check for whether the SSB object is returned, but also that the functionality it needs is included in it.
  2. For long-running processes (like Connections status polling) or anything with a callback, it is possible for the parent window to be closed between the time the callback was set up and when it runs. So the SSB object needs to be acquired anew (and error checking needs to happen) each time one of these runs.
  3. I know you wanted this done synchronously, but a lot of the places that need SSB could be vastly simplified with an asynchronous callback. As it is, there are a lot of setTimeout()s to just try again. I intentionally set this up so it was easy to convert over.
  4. The mutex system is localStorage-based. This is because it's very widely-supported across browsers, it uses the same browser API as the storage we're protecting in many cases, and per the localStorage spec browsers are required to implement mutexes in their implementations. That was tricky to get right, but there is a reason it delays startup by 2 seconds, and that's to check for pings from other tabs. Without that, it starts nearly immediately but does not properly protect against concurrent access.
KyleMaas commented 3 years ago

Also, if you can commit early and often, that'd be great, because my next step is to work on getting center clicks and context menus programmatically opening child windows so they can be tied in. I plan on doing the same. Don't need any more conflicts than necessary.

KyleMaas commented 3 years ago

Moved to #260.