Skip to content

Refactoring Event Handlers, make the code support for asynchronous request to prevent race-condition from Webhook #179

Merged
guzzilar merged 7 commits intomasterfrom
prevent-webhook-update-order-twice-approach-2
Jul 20, 2020
Merged

Refactoring Event Handlers, make the code support for asynchronous request to prevent race-condition from Webhook #179
guzzilar merged 7 commits intomasterfrom
prevent-webhook-update-order-twice-approach-2

Conversation

@guzzilar
Copy link
Copy Markdown
Contributor

1. Objective

In a race-condition where Omise Webhook is fired at the same time as buyer returns to the Callback URI (return_uri). A particular WooCommerce Order object will get updated its status twice which creates a consequence of a confirmation email is being sent to the recipient twice causing a confusion in a business flow that rely on this particular email.

Screen Shot 2563-07-15 at 01 52 59 copy

This happens because at the moment where Omise Webhook and Callback URI are triggered asynchronously at the same time, they both retrieve WC Order object with the pending status and update it to processing using WC_Order::payment_complete(), which triggers an email function to be executed twice.

This pull request is providing a solution to prevent the async request from creating a race-condition, by refactoring Event Handlers to make the code support for the Queue worker system in case if the race-condition happened.

Related information:
Related issue(s): T22125 (internal ticket)

2. Description of change

3. Quality assurance

🔧 Environments:

  • WooCommerce: v4.2.2.
  • WordPress: v5.4.2
  • PHP version: 7.3.3.

✏️ Details:

4. Impact of the change

There is a limitation of concurrent that Action Scheduler library can handle.
Please check the following document as reference: https://actionscheduler.org/perf

Action Scheduler will only process actions in a request until:
• 90% of available memory is used
• processing another 3 actions would exceed 30 seconds of total request time, based on the average processing time for the current batch
• in a single concurrent queue

Also

By default, Action Scheduler will only process actions for a maximum of 30 seconds in each request. This time limit minimises the risk of a script timeout on unknown hosting environments, some of which enforce 30 second timeouts.

By default, Action Scheduler will claim a batch of 25 actions. This small batch size is because the default time limit is only 30 seconds; however, if you know your actions are processing very quickly, e.g. taking microseconds not seconds, or that you have more than 30 second available to process each batch, increasing the batch size can slightly improve performance.

By default, Action Scheduler will run only one concurrent batches of actions. This is to prevent consuming a lot of available connections or processes on your webserver.

However, it shall not effect to its capability to handle the queue. Just for some high-volume transaction per minute website, it may take some seconds or minutes until all the queue get executed.

As stated in WP-Cron: https://developer.wordpress.org/plugins/cron/#what-is-wp-cron

5. Priority of change

Immediate.

6. Additional Notes

Just to clarify the relationship between 3 names.

  • WooCommerce integrated Action Scheduler library in the project
  • Action Scheduler is designed to work with WP-Cron
  • WP-Cron is a default built-in Cron worker that WordPress provided.

However, there is one point that we should understand of WP-Cron behaviour, to understand how the WC_Queue work.
As stated in WP-Cron document: https://developer.wordpress.org/plugins/cron/#what-is-wp-cron

WP-Cron works by checking, on every page load, a list of scheduled tasks to see what needs to be run. Any tasks due to run will be called during that page load.

WP-Cron does not run constantly as the system cron does; it is only triggered on page load.

With the system scheduler, if the time passes and the task did not run, it will not be re-attempted. With WP-Cron, all scheduled tasks are put into a queue and will run at the next opportunity (meaning the next page load). So while you can’t be 100% sure when your task will run, you can be 100% sure that it will run eventually.

The WP-Cron is only working when "any" of page get loaded. It's not time-based as a typical system scheduler.
Meaning that, even though we set some schedule to be executed after "5" mins, but if there is no one open "any" page of the WordPress website, then that schedule won't be triggered.

@guzzilar
Copy link
Copy Markdown
Contributor Author

Note: this approach is providing Queue Runner and Queueable classes. This will be useful for the case of implementing Scheduling feature (in the future) as well.

@guzzilar guzzilar force-pushed the prevent-webhook-update-order-twice-approach-2 branch 2 times, most recently from a5c4d32 to 441361f Compare July 17, 2020 11:48
return;
return true;
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removing all the code here as this event does not need any extra step/action from Webhook (however, still keep this handler class here to support those 3rd-party developers that may need to hook this Webhook event).

$event->resolve();
}
}
}
Copy link
Copy Markdown
Contributor Author

@guzzilar guzzilar Jul 17, 2020

Choose a reason for hiding this comment

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

I'm designing this class to be a place where we add code to be executed from queue-action hook.
i.e. it could have

public static function execute_subscription_payment( ) {
    // Perform OmiseSchedule
}

Comment thread includes/events/class-omise-event-charge-capture.php
Comment thread includes/events/class-omise-event-charge-capture.php
Copy link
Copy Markdown
Contributor

@mayurkathale mayurkathale left a comment

Choose a reason for hiding this comment

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

Few things in notes above.
I am still able to reproduce issue with second attempt of order payment. Tried first failed attempt with Alipay, then second successful attempt with Card payment which gives payment error note on callback page.

Copy link
Copy Markdown
Contributor

@mayurkathale mayurkathale left a comment

Choose a reason for hiding this comment

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

LGTM

guzzilar added 7 commits July 20, 2020 17:35
To make the payload data consistency on both Callback and Webhook data, this json_decode should be converting the payload data into array instead of object (as the same as OmiseCharge object).
…it is being used to handle a capture action from Omise Dashboard.
@guzzilar
Copy link
Copy Markdown
Contributor Author

@mayurkathale Thank you for the test and your review!

@guzzilar guzzilar merged commit 9135058 into master Jul 20, 2020
@guzzilar guzzilar deleted the prevent-webhook-update-order-twice-approach-2 branch July 20, 2020 11:26
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.

2 participants