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

Updates ioredis to ^5.0.4 #453

Closed
wants to merge 3 commits into from
Closed

Updates ioredis to ^5.0.4 #453

wants to merge 3 commits into from

Conversation

janthurau
Copy link
Collaborator

No description provided.

@janthurau
Copy link
Collaborator Author

Unfortunately Redlock is not really compatible yet, mike-marcacci/node-redlock#160

@kylealwyn
Copy link
Collaborator

Might be worth a fork? node-redlock might be a ghost town

@janthurau
Copy link
Collaborator Author

janthurau commented Jan 4, 2023

I'm thinking of just dropping locking there. Any opinions?

Scenarios that I have in my head:

  • When storing to a database, the locking would be moved to database responsibility, which shouldn't be an issue
  • When sending an HTTP / another message, each Hocuspocus instance could send the same message at the same time, but that might already happen due to timing issues, as the lock only prevents the hook from running at the exact same time.

If someone really needs to avoid this, they could just put the last store time in the ydoc and check that before doing anything expensive.

@jamespantalones
Copy link

@janthurau what are the downsides of removing the locks? if each instance sends the same message at the same time, could that cause clientside issues?

we are also looking into forking and dropping locks as we are running on a cluster, which seems to throw errors when a lock attempts to be acquired twice

https://github.com/mike-marcacci/node-redlock#using-clustersentinel

@lzj723
Copy link
Contributor

lzj723 commented Feb 19, 2023

Hi @jamespantalones , you can try below if you just want to solve require lock errors.
It's efficient for me though the code is not that succinct:

try {
  // try to get a lock
  const lock = await redlock.acquire(['xxx'], 2000);
  try {
    // do something with lock
  } catch (e) {
    console.error(`log something`, e);
  } finally {
    // Release the lock. And it may throw an ERROR too.
    try {
      await lock.release();
    } catch (e) {
      // console.error('lock release failed:', e);
    }
  }
} catch (e) {
  // It will throw an ERROR when require lock failed, so we need to catch it.
  console.warn('require lock failed:', e);
}

@janthurau janthurau force-pushed the feature/updatesIoredis branch from 6f71fa9 to dbcdfb8 Compare April 28, 2023 17:50
@janthurau
Copy link
Collaborator Author

I have released v2.1.0-alpha.0 which adds compatibility to ^5.0.4 and removes redlock. Please let me know if that breaks something

@janthurau janthurau changed the title Draft: Updates ioredis to ^5.0.4 Updates ioredis to ^5.0.4 Apr 28, 2023
@janthurau janthurau closed this Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants