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

feat(scripts): use redlock to prevent >1 instance of paypal-processor #12576

Closed
wants to merge 1 commit into from

Conversation

chenba
Copy link
Contributor

@chenba chenba commented Apr 19, 2022

Because:

  • we could easily end up running two instances of the paypal-processor
    during a deploy

This commit:

  • use a redis based distributed lock to ensure only one
    paypal-processor can run per env
  • add script options to control the lock name and duration, as well as
    completely bypassing the lock

@chenba chenba requested a review from a team as a code owner April 19, 2022 18:55
@chenba
Copy link
Contributor Author

chenba commented Apr 19, 2022

@bbangert one more time? I actually added redlock to the correct package this time. 😓

@@ -65,5 +121,6 @@ if (require.main === module) {
console.error(err);
process.exit(1);
})
.then((result) => process.exit(result));
.then((result) => process.exit(result))
.finally(() => lock?.release());
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Will a finally() run after a process.exit()? or is process.exit() immediate?
(although that should be a quick experiment for me to run in isolation separately)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah yeah, I think you are right that that finally won't get executed. I'll fix. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like it:

Promise.resolve("OK, computer")
  .then(msg => {
    console.log("success:", msg);
    throw new Error("Ruh roh");
  })
  .catch(err => {
    console.error("error:", err.message);
    process.exit(1);
  })
  .finally(() => console.log("I'm still here..."));
node --version # v16.14.2
node index.mjs
# success: OK, computer
# error: Ruh roh

Note that the .finally() log never seems to fire due to the hard process.exit().

Copy link
Contributor

@pdehaan pdehaan Apr 19, 2022

Choose a reason for hiding this comment

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

Deeper and deeper… This seems to do what I'd expect, but not sure if I know what I'm doing:

async function init(e=false) {
  if (e) {
    throw new Error("OH NO!");
  }
  return 0;
}

init(false)
  .then((result) => {
    console.log("then...", result);
    process.exitCode = result;
  })
  .catch((err) => {
    console.error("catch...", err.message);
    process.exitCode = 1;
  })
  .finally(() => {
    console.log("finally...", process.exitCode);
    process.exit(process.exitCode);
  });

I had to swap the .then() and .catch() order, since .catch() swallows errors then proceeds onto the next .then() chain.

But now if the initial init() or .then() block fails, the code should go into the .catch() block and set a non-zero exit code which falls through to the .finally() block which can release locks and set hard exit with your desired exit code.


UPDATE: Per the docs, https://nodejs.org/api/process.html#processexitcode, It doesn't sound like you even need to specify the redundant process.exitCode in the process.exit() command in our .finally() block above.

If code is omitted, exit uses either the 'success' code 0 or the value of process.exitCode if it has been set.

@chenba chenba force-pushed the fxa-4698-paypal-lock branch from 0331bce to cd13524 Compare April 19, 2022 19:57
.catch((err) => {
console.error(err);
process.exit(1);
exitStatus = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hhmm... Not sure this is needed since you already default it to 1 above on L119. 🤷
Unless we should be defaulting the exit status code to 0 above.

})
.then((result) => process.exit(result));
.finally(() => {
lock?.release();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance that the .release() API can throw an error?
Does this need to be wrapped in a try..catch for extra safety?

I guess the risk is that we make it to the finally() block and the lock release throws an error and our process.exit() never gets called (or would terminate with a 0 status code). Although maybe the thrown error would break elsewhere and it's a non-issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I moved the .release() call into init so that it's in a try block.

Because:
 - we could easily end up running two instances of the paypal-processor
   during a deploy

This commit:
 - use a redis based distributed lock to ensure only one
   paypal-processor can run per env
 - add script options to control the lock name and duration, as well as
   completely bypassing the lock
@chenba chenba force-pushed the fxa-4698-paypal-lock branch from cd13524 to 56ff4a5 Compare April 19, 2022 20:24
}
}

for await (const _ of processor.processInvoices()) {
Copy link
Member

Choose a reason for hiding this comment

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

If processing an invoice throws an exception, how is the lock released?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lock expires after DURATION since acquisition or the most recent extension.


for await (const _ of processor.processInvoices()) {
if (useLock && timer.elapsed() > Math.floor(lockDuration / 2)) {
await lock.extend(timer.elapsed());
Copy link
Member

Choose a reason for hiding this comment

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

The example shows:

lock = await lock.extend(timer.elapsed());

I pulled up the source code for extend, and it creates an entirely new lock object that is returned during extend:
https://github.com/mike-marcacci/node-redlock/blob/main/src/index.ts#L410

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh good catch. 😬 I'll assign the return value to lock.

@chenba
Copy link
Contributor Author

chenba commented Apr 20, 2022

Opened #12610 as an alternative approach with a auto-extending lock.

@chenba
Copy link
Contributor Author

chenba commented Apr 21, 2022

Closing this in favor of #12610. @bbangert @pdehaan thank you for all the feedback.

@chenba chenba closed this Apr 21, 2022
@chenba chenba deleted the fxa-4698-paypal-lock branch April 21, 2022 15:26
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.

3 participants