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

Fixes #4391 stop background processes before rollback and update #4392

Merged

Conversation

mostafa-hisham
Copy link
Contributor

Description

stop background processes before rollback and update to avoid fatal if the processes still running while the plugin changes

Fixes #4391

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Is the solution different from the one proposed during the grooming?

there is no grooming

How Has This Been Tested?

  • we should create an alpha release with this branch so we can test it in both rollback and upgrade

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@mostafa-hisham mostafa-hisham self-assigned this Sep 28, 2021
@mostafa-hisham mostafa-hisham requested a review from a team September 28, 2021 13:19
@mostafa-hisham mostafa-hisham marked this pull request as ready for review September 28, 2021 13:43
Copy link
Contributor

@iCaspar iCaspar left a comment

Choose a reason for hiding this comment

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

What's here looks good.
Can we add integration tests to cover this?

@mostafa-hisham
Copy link
Contributor Author

@iCaspar
I successfully created Integration tests for
stop CPCSS background Process
stop homepage preload background process

in both
stop warmup background process
stop sitemap preload background process

i think the problem is I couldn't start them in the integration test
to check if the upgrade or rollback action will stop them

you can have a look at the classes in the previous commit if you want to give it a try

Copy link
Contributor

@iCaspar iCaspar left a comment

Choose a reason for hiding this comment

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

Nice work.

mostafa-hisham and others added 2 commits September 30, 2021 15:54
Co-authored-by: Caspar Green <cg@caspar.green>
Co-authored-by: Caspar Green <cg@caspar.green>
@Tabrisrp Tabrisrp changed the title stop background processes before rollback and update Fixes #4391 stop background processes before rollback and update Oct 1, 2021
@piotrbak piotrbak self-assigned this Oct 5, 2021
@piotrbak
Copy link
Contributor

piotrbak commented Oct 5, 2021

@mostafa-hisham
When rolling back from the branch to 3.9, there's no fatal error, but the RUCSS process can be stuck on warming up with wrong data:
"warmup_status":{"total":1,"warmed_count":0,"notwarmed_resources":[],"duration":70,"completed":false}.
While updating, it got stuck too, but the data was more accurate.

It seems not to be a problem with this branch, as it's only here to prevent fatal error from happening. There's no pattern to reproduce this though.

@Tabrisrp What do you think? Should we somehow make sure that RUCSS process will be continued and more reliable in this PR?

@Tabrisrp
Copy link
Contributor

Tabrisrp commented Oct 5, 2021

Since we cancel the process, we can't resume it easily after. I'd say it's ok for this PR, and we can follow up on another issue to handle this case.

@piotrbak
Copy link
Contributor

piotrbak commented Oct 5, 2021

@mostafa-hisham @Tabrisrp After one of the tries, my log got blasted with that kind of notices:

[05-Oct-2021 15:40:59 UTC] PHP Notice:  Undefined index: external in /var/www/rocketlab.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/RUCSS/Warmup/ResourceFetcherProcess.php on line 123

[05-Oct-2021 15:40:59 UTC] PHP Notice:  Undefined index: path in /var/www/rocketlab.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/RUCSS/Warmup/ResourceFetcherProcess.php on line 123

[05-Oct-2021 15:40:59 UTC] PHP Notice:  Undefined index: path in /var/www/rocketlab.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/RUCSS/Warmup/ResourceFetcherProcess.php on line 135

Do you think we can do something about that?

@mostafa-hisham
Copy link
Contributor Author

@piotrbak @Tabrisrp
i guarded the variables to avoid warnings

Copy link
Contributor

@piotrbak piotrbak left a comment

Choose a reason for hiding this comment

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

@Tabrisrp Tabrisrp merged commit 31617ea into develop Oct 8, 2021
@Tabrisrp Tabrisrp deleted the fix/4391-disable-background-processes-on-upgrade-rollback branch October 8, 2021 15:29
@piotrbak piotrbak added this to the 3.10.1 milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment