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/mutex lock #911

Merged
merged 5 commits into from
Aug 23, 2023
Merged

Fix/mutex lock #911

merged 5 commits into from
Aug 23, 2023

Conversation

alexanderleegs
Copy link
Contributor

@alexanderleegs alexanderleegs commented Aug 18, 2023

Problem

This PR fixes issues with our existing mutex lock. It has been tested on both local dev and staging.

Currently, our route handler behaviour seems to suffer from 2 issues that make our mutex locks not work as intended:

  • The wrapped route is not awaited, resulting in the existing behaviour where the mutex locks and unlocks while the method is still executing
  • Express' next does not cut off the remainder of the method, and must be cut off explicitly - in our existing implementation, even if the mutex lock is hit, the actual route still continues to execute

This PR modifies the behaviour of both our write and rollback wrappers to handle this scenario.

In addition, the returned error has been modified from 409 to 423 (Locked) - this is to better reflect the reason for rejecting the request, and also results in a regular error toast on the frontend instead of the special conflict modal that would normally show up for modification of outdated files.

Resolves IS-478

@alexanderleegs alexanderleegs requested a review from a team August 18, 2023 09:22
Copy link
Contributor

@harishv7 harishv7 left a comment

Choose a reason for hiding this comment

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

@alexanderleegs have we retried how this functions with FE with the new 423 error?

@alexanderleegs
Copy link
Contributor Author

@alexanderleegs have we retried how this functions with FE with the new 423 error?

Yup, it shows up as an error toast with the message we've specified (Your page could not be updated. Someone else is currently modifying repo a-test-v4. Please try again later.)

* feat: add hasGitFileLock

* feat: make check for git file lock in route handlers

* fix: always return true if something exists on the path

* fix: return if handleGitFileLock fails

* fix: use lockedError instead of conflictError

* chore: update error message
@alexanderleegs alexanderleegs merged commit 49da69c into develop Aug 23, 2023
@mergify mergify bot deleted the fix/mutex-lock branch August 23, 2023 02:23
@harishv7 harishv7 mentioned this pull request Aug 23, 2023
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.

2 participants