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

Fix locking race condition #557

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mohsin-r
Copy link
Member

@mohsin-r mohsin-r commented Feb 21, 2025

Changes

  • Fixed an issue where one storyline could acquire the secret key of a different storyline due to a race condition, which would prevent it from saving itself.

Notes

Unfortunately this was being caused by broadcasting the lock/unlock of a storyline to all clients. I believe we had this as a benefit that all clients would be able to know when a storyline was locked/unlocked. However, in certain situations, a client would take the secret key of another client instead of the one they requested. Consider the following example:

  • Client A connects to the wss.
  • Client B connects to the wss.
  • Client A requests to lock storyline X.
  • Client B requests to lock storyline Y.
  • Server broadcasts to all clients that X is locked, along with its secret.
  • Client A responds by setting X's secret in the store and stops listening for a response.
  • Client B responds by setting X's secret in the store and stops listening for a response.
  • Server broadcasts to all clients that Y is locked, along with its secret.
  • Nothing happens since both clients already have a "secret".

We can see here that the result is dependent on the order of operations, and when you have multiple clients, as was the case in the testing party, chaos ensues!

The easiest fix was for the wss to respond only to client requesting the storyline lock/unlock and change a few order of operations. Unfortunately this does mean we lose out on the benefit stated above, but it does remove the possibility of any race conditions. In case we want to add it back, I have opted to comment it out instead of removing it altogether.

Testing

Steps:

  1. Open the dev site in several tabs (or partner up with someone on the team).
  2. Attempt to load and save several different storylines simultaneously or as close to each other as possible.
  3. Ensure everything is respectful.
  4. Feel free to also verify that timeouts work.

This change is Reviewable

Copy link

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/socket-issues

Copy link
Member

@IshavSohal IshavSohal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mohsin-r)

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mohsin-r)

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mohsin-r)

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

Successfully merging this pull request may close these issues.

4 participants