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: Invoice number race condition #669

Merged
merged 12 commits into from
Dec 5, 2023
Merged

Conversation

alexmigf
Copy link
Member

@alexmigf alexmigf commented Dec 5, 2023

closes #619

This PR attempts to fix the race condition issue spotted using Stripe plugin.

It changes the lock time to 60 seconds, to avoid expiring before the code being finished, and now also adds a lock to the email attachment function as well, which is the primary way of getting duplicated numbers.

includes/class-wcpdf-main.php Show resolved Hide resolved
includes/class-wcpdf-main.php Outdated Show resolved Hide resolved
@alexmigf
Copy link
Member Author

alexmigf commented Dec 5, 2023

@MohamadNateqi check now

includes/class-wcpdf-main.php Outdated Show resolved Hide resolved
Copy link
Contributor

@MohamadNateqi MohamadNateqi left a comment

Choose a reason for hiding this comment

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

LGTM!

includes/class-wcpdf-main.php Outdated Show resolved Hide resolved
Copy link
Contributor

@MohamadNateqi MohamadNateqi left a comment

Choose a reason for hiding this comment

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

All good!

Copy link
Contributor

@MohamadNateqi MohamadNateqi left a comment

Choose a reason for hiding this comment

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

The last commit was great.

What do you think of checking that setting here and sending an empty array if it's not checked, instead of adding that in the Semaphore class? Then the log() method can only check the emptiness of that property.

$lock = new Semaphore( $this->lock_name, $this->lock_time, array( wc_get_logger() ), $this->lock_context );

Copy link
Contributor

@MohamadNateqi MohamadNateqi left a comment

Choose a reason for hiding this comment

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

Perfect!

@alexmigf alexmigf merged commit dbba098 into main Dec 5, 2023
5 checks passed
@alexmigf alexmigf deleted the 619-invoice-number-race-condition branch December 5, 2023 17:05
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.

Avoid double invoice creation on race condition related with Stripe payments
2 participants