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 release on expired locks #180

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

Conversation

phil-r
Copy link

@phil-r phil-r commented Jun 25, 2022

This PR:

Note:

  • Some tests seem to be non-predictable, e.g. multi › cluster - acquires, extends, and releases a single lock fails with The lock value was incorrect., but it seems to happen outside of this PR as well
  • Better way would be to actually try to expire expired locks (👀), just to be sure(basically ignore count from RELEASE_SCRIPT). But I couldn't find an elegant way to do that

@phil-r
Copy link
Author

phil-r commented Jun 29, 2022

Sadly, multi › cluster - acquires, extends, and releases a single lock failed, but I'm pretty sure it's unrelated to this PR,as it fails on most new PRs

@phil-r
Copy link
Author

phil-r commented Oct 14, 2023

Hey @VdmPro I don't think it's a problem with node-redlock or this PR, your code is "destined" to fail.

by default there are 10 retries with 200ms delay, so if lock is not acquired in ~2s it will fail. Your code will take 6000ms at best (30 tries * 200ms sleep).

There are 2 ways to address this:

  1. Adjust number of retries and delay between them:
const tries = 30;
for (let i = 0; i < tries; i++) {
    redlock
        .using(['test'], 10_000, {
            automaticExtensionThreshold: 200,
            retryDelay: 200,
            retryCount: tries * 2,
          }, async () => {
            await sleep(200);
            console.log('OK');
        })
        .catch((e) => console.error(e.message));
}
  1. Run "tasks" sequentially (by adding await before each redlock.using:
for (let i = 0; i < 30; i++) {
   await redlock
        .using(['test'], 10_000, async () => {
            await sleep(200);
            console.log('OK');
        })
        .catch((e) => console.error(e.message));
}

There is not much point in using redlock in this case though 🤔

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.

Unable to reach quorum
2 participants