Background process runner #512

ankitrox · 2022-09-05T13:49:26Z

Summary

Create a background process runner class. This is mainly responsible for:

Handling the background job call via ajax action.
Validating the parameters.
Running the process.
Watching the process for time and memory exhaustion.
Terminating the process.
Error handling.

Fixes #482 #493 #494

Relevant technical choices

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

…cess-runner

To-do: Add partial status in job class

…cess-runner

jjgrainger

@ankitrox Looking good so far. I've left a couple of comments to address.

Additionally, there are a couple of other things. One is the failing tests on this PR the second is the inclusion of the Job class which is also in #507 and should not be present in this PR.

jjgrainger · 2022-09-08T12:32:08Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+	 *
+	 * @var string Background process action name.
+	 */
+	const BG_PROCESS_ACTION = 'background_process_handle_request';


@ankitrox we should prefix this action name with perflab_

Suggested change

const BG_PROCESS_ACTION = 'background_process_handle_request';

const BG_PROCESS_ACTION = 'perflab_background_process_handle_request';

jjgrainger · 2022-09-08T12:38:15Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+			$max_execution_time = ( ! empty( $time ) && ( $time > $min_execution_time ) ) ? $time - 10 : $min_execution_time;
+		}
+
+		return ( $current_time >= ( $run_start_time + $max_execution_time ) );


@ankitrox we should also use apply_filters on the value returned here as we have with the memory limit. The filter should be perflab_background_process_time_exceeded

Suggested change

return ( $current_time >= ( $run_start_time + $max_execution_time ) );

$time_exceeded = ( $current_time >= ( $run_start_time + $max_execution_time ) );

return apply_filters( 'perflab_background_process_time_exceeded', $time_exceeded );

jjgrainger · 2022-09-08T12:49:29Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+	 * @return void
+	 */
+	public function handle_request() {
+		try {


@ankitrox I feel like this method and try block does too much? I think we can split some of the logic between the handle_request() and run() methods. I think this because we have exceptions thrown that don't get caught as the job is not available.

The way I see it, handle_request() should verify and determine the request is valid. If not, the method should return early, or maybe even return a wp_send_json_error error message? If the request is valid it then calls the run() method,

Then the run method is responsible for running the job and I think this could contain everything from the $this->job->lock() line down.

jjgrainger · 2022-09-08T12:50:53Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+			 * @param array $data Job data.
+			 */
+			do_action( 'perflab_job_' . $this->job->get_name(), $this->job->get_data() );
+		} while ( ! $this->memory_exceeded() && ! $this->time_exceeded() );


@ankitrox we should also check if the job is marked as complete here too.

Suggested change

} while ( ! $this->memory_exceeded() && ! $this->time_exceeded() );

} while ( ! $this->memory_exceeded() && ! $this->time_exceeded() && ! $this->job->is_completed() );

jjgrainger · 2022-09-08T12:53:44Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+		 * Do not call the background process from within the script if the
+		 * real cron has been setup to do so.
+		 */
+		if ( defined( 'ENABLE_BG_PROCESS_CRON' ) ) {


@ankitrox I think the configuration we have defined so far is to disable this background process entirely with a DISABLE_BACKGROUND_PROCESS constant. I don't believe we have the infrastructure defined to have the WP Cron handle the additional batches.

I think we should remove this here and consider where the DISABLE_BACKGROUND_PROCESS check would need to be made.

We will also have this constant to let the real cron job call background process. In such cases, script would not call next batch of the job from within itself.
This is discussed with @felixarntz in alternate approaches to improve background process workflow.

Thanks @ankitrox I'm not sure how this works in the grand scheme of things as we haven't defined or built the part that handles subsequent batches. I think we can approach this change in 1 of 2 ways.

Keep the constant as part of this PR, but make a change as this is used twice in the code, once here and again inside the perflab_dispatch_background_process_request. I think we should aim to only have to check this constant once.

Remove the constant from the PR and we can add it as part of a new PR and stream of work to introduce this functionality.

I think in both cases we should also create a new GitHub issue with acceptance criteria to outline the work.

cc @felixarntz

…cess-runner

ankitrox · 2022-09-12T14:08:57Z

@ankitrox Looking good so far. I've left a couple of comments to address.

Additionally, there are a couple of other things. One is the failing tests on this PR the second is the inclusion of the Job class which is also in #507 and should not be present in this PR.

@jjgrainger Let's aim to get background class PR merge first. As process runner is using job instance, I have merged it in this PR. Once background job class is merged, we can get this PR ready for review to others.

…cess-runner

Filter to update expiry days for job cleanup

…cess-runner

…process-runner

jjgrainger · 2022-10-04T15:37:11Z

@ankitrox I think this PR is becoming too large and tackling too many parts of the background process.

For starters, I would argue the work for status cron #484 can be moved out of the background process runner and into its own PR/file/class. I think this should definitely be moved out of the PR and into a new one focused on fixing that specific issue.

Additionally, this PR seems to also fix #480 #481 and even #490 #483, which were already being addressed in other PR's. We want to keep these PR's smaller to make the review process more focused and less complex.

Can you update this PR to focus on the acceptance criteria in #482 #493 #494.

Let me know if you have any questions.

ankitrox · 2022-10-05T10:42:02Z

@ankitrox I think this PR is becoming too large and tackling too many parts of the background process.

For starters, I would argue the work for status cron #484 can be moved out of the background process runner and into its own PR/file/class. I think this should definitely be moved out of the PR and into a new one focused on fixing that specific issue.

Additionally, this PR seems to also fix #480 #481 and even #490 #483, which were already being addressed in other PR's. We want to keep these PR's smaller to make the review process more focused and less complex.

Can you update this PR to focus on the acceptance criteria in #482 #493 #494.

Let me know if you have any questions.

Thanks @jjgrainger
I have removed the cron related code from PR. I will create a new PR for that one.
Also, updated the dependent issue in PR description.

jjgrainger

Thanks @ankitrox left some comments but looking good so far

jjgrainger · 2022-10-05T14:46:04Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+	public function __construct() {
+		// Handle job execution request.
+		add_action( 'wp_ajax_' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) );
+		add_action( 'wp_ajax_nopriv_' . Perflab_Background_Process::BG_PROCESS_ACTION, array( $this, 'handle_request' ) );


@ankitrox we should remove this action as we do not want unauthenticated requests triggering the background process. We should stick with the wp_ajax_perflab_background_process_handle_request action only.

jjgrainger · 2022-10-05T14:49:05Z

modules/images/regenerate-existing-images/helper.php

+ *
+ * @param int $job_id Job ID.
+ */
+function perflab_dispatch_background_process_request( $job_id ) {


@ankitrox based on the acceptance criteria in #480 this function should be named perflab_start_background_job

jjgrainger · 2022-10-05T14:52:01Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+			return;
+		}
+
+		perflab_dispatch_background_process_request( $job_id );


@ankitrox we are handling next batch requests differently to the start request. See #490 for the AC's here. The function name that should be called is perflab_background_process_next_batch.

There's only a difference of $key in start_job and next_batch functions. I think we can somehow combine/compose these functions to avoid rewriting entire thing twice.

jjgrainger · 2022-10-05T15:08:58Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+		 * Do not call the background process from within the script if the
+		 * real cron has been setup to do so.
+		 */
+		if ( defined( 'ENABLE_BG_PROCESS_CRON' ) ) {


Thanks @ankitrox I'm not sure how this works in the grand scheme of things as we haven't defined or built the part that handles subsequent batches. I think we can approach this change in 1 of 2 ways.

Keep the constant as part of this PR, but make a change as this is used twice in the code, once here and again inside the perflab_dispatch_background_process_request. I think we should aim to only have to check this constant once.

Remove the constant from the PR and we can add it as part of a new PR and stream of work to introduce this functionality.

I think in both cases we should also create a new GitHub issue with acceptance criteria to outline the work.

cc @felixarntz

jjgrainger · 2022-10-05T15:43:25Z

...es/images/regenerate-existing-images/background-process/class-perflab-background-process.php

+	 *
+	 * @throws Exception Invalid request for background process.
+	 */
+	public function handle_request() {


@ankitrox I think this method does too much and should only be responsible for handling the HTTP request and calling the run() method. I would make the following changes:

Remove the try/catch block and exceptions - as we're handling the HTTP request we can return early with a error response. This could be a combination of using WP_Error instead of Exception and returning it with wp_send_json_error

The final part of the handle_request() method should be the $this->run() call.

The unlock() and next_batch() lines should be moved to the end of the run() method. Reasoning here is that this logic is part of the job running processes. If we were to trigger running a job via a different method (WP_Cron) calling $this->run() would still work as expected.

If you wanted you can use a try/catch block inside the run() method to catch any system errors while jobs are running and handle those properly.

Let me know if this makes sense, happy to discuss on a call

Made the proposed changes.
I think adding try catch to run method would make more sense when we will implement error handling.

ankitrox · 2022-10-05T18:08:49Z

Thanks @ankitrox I'm not sure how this works in the grand scheme of things as we haven't defined or built the part that handles subsequent batches. I think we can approach this change in 1 of 2 ways.

Keep the constant as part of this PR, but make a change as this is used twice in the code, once here and again inside the perflab_dispatch_background_process_request. I think we should aim to only have to check this constant once.

Remove the constant from the PR and we can add it as part of a new PR and stream of work to introduce this functionality.

I think in both cases we should also create a new GitHub issue with acceptance criteria to outline the work.

cc @felixarntz

@jjgrainger Regarding this comment, I think option #2 is better; to handle this in separate issue and PR.

adamsilverstein · 2024-05-24T18:45:19Z

@ankitrox looks like this PR has gone stale, can we go ahead and close it?

github-actions · 2024-05-24T18:45:32Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @ankit.gade@rtcamp.com, @ankitrox.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: ankit.gade@rtcamp.com, ankitrox.

Co-authored-by: jjgrainger <joegrainger@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: eugene-manuilov <manuilov@git.wordpress.org>
Co-authored-by: bethanylang <mxbclang@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

adamsilverstein

is this still needed?

westonruter · 2024-05-24T19:18:58Z

@adamsilverstein looking back at old PRs from this time period, it seems the main push was to add image regeneration as a background process which this PR would serve as infrastructure for. It seems this topic hasn't been on the radar for quite some time, so potentially several such PRs should be closed. Not sure why that got deprioritized.

Background process runner class

ae86c66

ankitrox self-assigned this Sep 5, 2022

Ankit Gade added 18 commits September 5, 2022 19:59

Memory check for process

ca2b628

Fix: Action name for non-logged in user

dc08469

Tests: constant and hook

7c10ad2

Code commenting

076a5bb

Fix: phpcs

d1f0f63

Tests: handle request

e3c7381

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

bf2bc3d

…cess-runner

Change job status when ran successfully

a7c0150

To-do: Add partial status in job class

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

fbbeb2e

…cess-runner

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

f5a5f9b

…cess-runner

Set partial status for successful batch

8d85858

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

6aca9d2

…cess-runner

Fix: tests

a0b78d7

Update method docs

6f4682f

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

96aa1f1

…cess-runner

Fix: tests

439930b

Method to call next batch of jobs

6c50fe2

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

a15a608

…cess-runner

jjgrainger requested changes Sep 8, 2022

View reviewed changes

Ankit Gade added 4 commits September 8, 2022 23:55

Fix failing test

31d75f4

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

0630872

…cess-runner

Execute job till completed

3dec70e

Fix: while loop condition

230c694

Ankit Gade added 8 commits September 23, 2022 19:04

Docblock updates

fc1dfdf

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

dfa4cad

…cess-runner

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

b0fa309

…cess-runner

Fix: method name for job action

0f9f459

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

b50a436

…cess-runner

Docblock updates

9fa957c

Filter to update expiry days for job cleanup

Merge branch 'experiment/background-job-class' into experiment/bg-pro…

d0dbc86

…cess-runner

Merge branch 'feature/regenerate-existing-images' into experiment/bg-…

c7558f2

…process-runner

Ankit Gade added 3 commits October 4, 2022 22:23

Remove irrelevant code

8aee2e2

Helper method to get job instance

923142b

Tests: perflab_get_background_job

38d1596

ankitrox marked this pull request as ready for review October 5, 2022 10:40

Tests: get background job

8029a90

ankitrox requested review from felixarntz, eugene-manuilov and mukeshpanchal27 October 5, 2022 12:51

jjgrainger requested changes Oct 5, 2022

View reviewed changes

Refactor handle_request method

e0213ed

Ankit Gade added 2 commits October 6, 2022 18:58

Refactor request triggering function

8c5dbab

Tests: start process and next batch process

f63e59f

ankitrox requested a review from adamsilverstein October 6, 2022 16:25

Tests: helper functions for batch call

d517093

ankitrox requested a review from jjgrainger October 6, 2022 16:28

adamsilverstein reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background process runner #512

Background process runner #512

ankitrox commented Sep 5, 2022 •

edited

jjgrainger left a comment

jjgrainger Sep 8, 2022

jjgrainger Sep 8, 2022

jjgrainger Sep 8, 2022

jjgrainger Sep 8, 2022

jjgrainger Sep 8, 2022

ankitrox Sep 12, 2022

jjgrainger Oct 5, 2022

ankitrox commented Sep 12, 2022

jjgrainger commented Oct 4, 2022

ankitrox commented Oct 5, 2022

jjgrainger left a comment

jjgrainger Oct 5, 2022

jjgrainger Oct 5, 2022

jjgrainger Oct 5, 2022

ankitrox Oct 5, 2022

jjgrainger Oct 5, 2022

jjgrainger Oct 5, 2022

ankitrox Oct 6, 2022 •

edited

ankitrox commented Oct 5, 2022 •

edited

adamsilverstein commented May 24, 2024

github-actions bot commented May 24, 2024 •

edited

adamsilverstein left a comment

westonruter commented May 24, 2024

	const BG_PROCESS_ACTION = 'background_process_handle_request';
	const BG_PROCESS_ACTION = 'perflab_background_process_handle_request';

	return ( $current_time >= ( $run_start_time + $max_execution_time ) );
	$time_exceeded = ( $current_time >= ( $run_start_time + $max_execution_time ) );
	return apply_filters( 'perflab_background_process_time_exceeded', $time_exceeded );

	} while ( ! $this->memory_exceeded() && ! $this->time_exceeded() );
	} while ( ! $this->memory_exceeded() && ! $this->time_exceeded() && ! $this->job->is_completed() );

Background process runner #512

Are you sure you want to change the base?

Background process runner #512

Conversation

ankitrox commented Sep 5, 2022 • edited

Summary

Relevant technical choices

Checklist

jjgrainger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitrox commented Sep 12, 2022

jjgrainger commented Oct 4, 2022

ankitrox commented Oct 5, 2022

jjgrainger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitrox Oct 6, 2022 • edited

Choose a reason for hiding this comment

ankitrox commented Oct 5, 2022 • edited

adamsilverstein commented May 24, 2024

github-actions bot commented May 24, 2024 • edited

Unlinked Accounts

adamsilverstein left a comment

Choose a reason for hiding this comment

westonruter commented May 24, 2024

ankitrox commented Sep 5, 2022 •

edited

ankitrox Oct 6, 2022 •

edited

ankitrox commented Oct 5, 2022 •

edited

github-actions bot commented May 24, 2024 •

edited