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

Add locking in the app to avoid deadlock #123

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

knolleary
Copy link
Member

Fixes #122

As described in the linked issue, a deadlock is possible if we get parallel update requests for different scopes under the same instance.

A full fix would require adding a new table to the database against which we obtain the lock for the instance (rather then the existing instance/scope level lock).

I've chosen a more pragmatic solution to get this fixed quickly; rather than make db changes, this uses an in-memory mutex that marshals access to the instance.

This isn't a scalable solution as the locks are held in memory of the app (so no horizontal scaling possible); but it will work within our current usage.

I've also simplified the code that was calculating the current context usage. The previous code was retrieving all of the contents and then calculating its size. This can be done in a single sql query - no need to transfer all the data over the network each time. I've tested this against postgres and sqlite.

@knolleary knolleary requested a review from Steve-Mcl August 8, 2024 13:50
Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

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

I have gone over this with a fine tooth comb. Apart from line 153 where the transaction is not included in the call to this.quota, the idea is sound.

While not including the transaction object may typically not cause any issues I have witnessed this in other places of the code with sequelize where the active transaction was not included in a query caused issues.

As for the introduction in-mem marshalling the code looks good and should contain the issue at hand. In particular I note the unlock call is in a finally block with all of the "locking" code contained within the try block.

@knolleary knolleary merged commit 7dc5b32 into main Aug 8, 2024
3 checks passed
@knolleary knolleary deleted the 122-fix-deadlock branch August 8, 2024 15:33
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.

Quota handling can lead to deadlock under load
2 participants