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(server): wrap read-modify-write apis with distributed lock #5979

Merged
merged 23 commits into from
Mar 15, 2024

Conversation

darkskygit
Copy link
Member

@darkskygit darkskygit commented Mar 1, 2024

impl new distributed lock with using syntax:

{
   // lock is acquired here
   await using lock = await mutex.lock('resource-key');
   if (lock) {
     // do something
   } else {
     // failed to lock
   }
}
// lock is released here

Copy link
Member Author

darkskygit commented Mar 1, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @darkskygit and the rest of your teammates on Graphite Graphite

Copy link

nx-cloud bot commented Mar 1, 2024

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 79.80535% with 83 lines in your changes are missing coverage. Please review.

Project coverage is 61.32%. Comparing base (7fdb1f2) to head (01b39ba).

Files Patch % Lines
packages/backend/server/src/plugins/redis/mutex.ts 41.66% 56 Missing ⚠️
.../server/src/core/workspaces/resolvers/workspace.ts 72.72% 21 Missing ⚠️
...ages/backend/server/src/plugins/redis/instances.ts 57.14% 3 Missing ⚠️
packages/backend/server/src/core/quota/service.ts 85.71% 1 Missing ⚠️
...server/src/fundamentals/error/too-many-requests.ts 92.85% 1 Missing ⚠️
packages/backend/server/src/plugins/redis/index.ts 96.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           canary    #5979      +/-   ##
==========================================
+ Coverage   61.02%   61.32%   +0.29%     
==========================================
  Files         488      492       +4     
  Lines       22115    22447     +332     
  Branches     1944     1959      +15     
==========================================
+ Hits        13496    13765     +269     
- Misses       8390     8453      +63     
  Partials      229      229              
Flag Coverage Δ
server-test 70.47% <79.80%> (+0.23%) ⬆️
unittest 40.44% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@darkskygit darkskygit requested a review from forehalo March 1, 2024 08:27
Copy link
Member

forehalo commented Mar 1, 2024

this is a bad idea, make all the db queries must be aware of query engine.

@github-actions github-actions bot added the test Related to test cases label Mar 1, 2024
@darkskygit
Copy link
Member Author

this is a bad idea, make all the db queries must be aware of query engine.

currently prisma only support get a transaction by Prisma.$transaction

but i just found a prisma transaction plugin for nestjs, what are your thoughts on this usage?

@forehalo
Copy link
Member

forehalo commented Mar 1, 2024

what about a resource lock for mutating data, for example:

function invite(workspaceId, userId) {
  // this is good because it tell us there may be concurrency issue if the resource not locked
  // we can safely loop query the lock in a given time limit like 100ms(is 100ms enough for any table mutating?), and block the lock until the last use get released.
  // if the time limit passes by, force rewrite the lock and go ahead
  const lock = await this.mutex.lock('members:' + workspaceId)
  
  try {
    await addUserToWorkspace(workspaceId, userId)
  } finally {
    lock.release()
  }

  // ...or
  await this.mutex.lock('members:' + workspaceId, async () => {
  //                    ^ maybe strong typed so we won't get lost if different callers request for a same lock
    await addUserToWorkspace(workspaceId, userId)
  })
}

@darkskygit
Copy link
Member Author

darkskygit commented Mar 1, 2024

what about a resource lock for mutating data, for example:

function invite(workspaceId, userId) {
  // this is good because it tell us there may be concurrency issue if the resource not locked
  // we can safely loop query the lock in a given time limit like 100ms(is 100ms enough for any table mutating?), and block the lock until the last use get released.
  // if the time limit passes by, force rewrite the lock and go ahead
  const lock = await this.mutex.lock('members:' + workspaceId)
  
  try {
    await addUserToWorkspace(workspaceId, userId)
  } finally {
    lock.release()
  }

  // ...or
  await this.mutex.lock('members:' + workspaceId, async () => {
  //                    ^ maybe strong typed so we won't get lost if different callers request for a same lock
    await addUserToWorkspace(workspaceId, userId)
  })
}

if we use mutex, then it needs to be a distributed lock
invite include querying and writing to multiple table, directly locking the entire resource can indeed make the logic simpler

but for single table read-modify-write(e.g.

), i still prefer use transaction, that allow database to optmizie queries

@darkskygit darkskygit force-pushed the darksky/mutate-db-with-transaction branch from b2d7731 to e5a6676 Compare March 1, 2024 13:52
@darkskygit darkskygit requested review from forehalo and removed request for forehalo March 4, 2024 06:20
@darkskygit darkskygit force-pushed the darksky/mutate-db-with-transaction branch 2 times, most recently from 7726607 to 256a053 Compare March 4, 2024 09:43
@darkskygit darkskygit changed the title feat: wrap read-modify-write apis with transaction feat: wrap read-modify-write apis with distributed lock Mar 4, 2024
@darkskygit darkskygit force-pushed the darksky/mutate-db-with-transaction branch from 256a053 to 858bf26 Compare March 5, 2024 05:18
@darkskygit darkskygit force-pushed the darksky/mutate-db-with-transaction branch from 858bf26 to df14857 Compare March 5, 2024 09:58
@darkskygit darkskygit requested a review from forehalo March 5, 2024 10:17
@darkskygit darkskygit force-pushed the darksky/mutate-db-with-transaction branch from f0c3aed to 5c5208b Compare March 5, 2024 10:31
@darkskygit
Copy link
Member Author

darkskygit commented Mar 5, 2024

consider that the auth module is refacting, restack the throttler refactor branch and develop it based on the auth branch but not this branch

@Brooooooklyn Brooooooklyn changed the title feat: wrap read-modify-write apis with distributed lock fix(server): wrap read-modify-write apis with distributed lock Mar 11, 2024
@Brooooooklyn
Copy link
Member

How about expose using API:

await using _ = await this.mutex.lock(`key`);

@darkskygit
Copy link
Member Author

How about expose using API:

await using _ = await this.mutex.lock(`key`);

looks more elegant than the existing solution, I will add support for this usage

@darkskygit darkskygit force-pushed the darksky/mutate-db-with-transaction branch from 80131f3 to ac3a161 Compare March 11, 2024 13:28
@darkskygit darkskygit merged commit 34f892b into canary Mar 15, 2024
38 of 42 checks passed
@darkskygit darkskygit deleted the darksky/mutate-db-with-transaction branch March 15, 2024 05:31
Brooooooklyn added a commit that referenced this pull request Mar 15, 2024
@forehalo forehalo restored the darksky/mutate-db-with-transaction branch March 15, 2024 09:06
@darkskygit darkskygit deleted the darksky/mutate-db-with-transaction branch March 15, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:server test Related to test cases
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants