Skip to content

Avoid checking for plugin updates in each page refresh #769

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

Open
wants to merge 1 commit into
base: bugfix
Choose a base branch
from

Conversation

pupi1985
Copy link
Contributor

I'm sending this to the bugfix branch as it is a minor feature that doesn't really need to wait for 1.9 and could serve as an excuse to release a new patch version soon.

@svivian
Copy link
Collaborator

svivian commented Jul 31, 2020

Left this a bit too long so had to merge into dev. However, I noticed that it only shows updates on the first page load of the plugins page. When you go back to the page the updates are missing, as the 'last_plugin_update_check' option gets updated and it doesn't check again.

We ought to store a list of the actual updates required - that way we can show updates every time but without all the AJAX requests.

@pupi1985
Copy link
Contributor Author

pupi1985 commented Aug 1, 2020

In order to accomplish this, it is needed to identify each plugin and store for each of them the latest version that has been detected as an update. Having a plugin option per plugin will pollute the ^options table. Having one plugin option to store these as key-value pairs in a JSON object seems to be a good way to go.

This type of storage will require for each plugin that needs to be added to the list to:

  1. load the option
  2. update it with the update version data
  3. save the option.

However, there is a concurrency issue here. There are more than une version updates running at the same time. That means the options table will be loaded twice (once per thread) before it gets updated by any/some of them. Then each thread will update the table without contemplating the result from the others. Example:

  1. The option is empty
  2. Plugin A and B start the version check and fetch the empty list from the database options using qa_opt()
  3. Plugin A detects an update, appends itself to the option content which is empty and finally saves the option to the database
  4. Plugin B detects an update, appends itself to the option content which is empty and finally saves the option to the database

Result: only plugin B has an update. So here it is needed to override the qa_opt cache and hit the database directly. The core doesn't not have such a function but I've faced a few other situations where this is needed. So I think it makes sense to add it.

The cleanup of the option (for removed or already updated plugins) will happen once the full check happens again.

Does this all make sense?

PS: I didn't get so had to merge into dev. I don't see this has been merged. I can rebase this on top of bugfix or dev, whatever you prefer.

@svivian
Copy link
Collaborator

svivian commented Aug 2, 2020

I think the concurrency issue can be solved by waiting for all the AJAX requests to finish, then doing one update with all the version numbers. jQuery has some integration with Promises so should be able to do this.

However another option might be to use a separate qa_plugins table, with currentversion and latestversion fields. That way each AJAX request can update one row. Can also store the ‘active’ state in there.

I don't see this has been merged.

See the dev branch, third one down as of writing. I’ve recently moved some more pages over to the new controller system so had to merge into Controllers/Admin/Plugins.php

@pupi1985
Copy link
Contributor Author

You're right: all those approach would solve the issue. However, I'm not sure they are the best way to go.

Using promises as a barrier would result in the need to send all the plugin data that needs to be stored in the database to the client. That way, the client will perform a last action which would be asking the server to perform the database update. This implies changing the controllers so that they send the information to the client but also implementing the new controller (or workflow step) of receiving the final update.

Having an additional table for storing a few pieces of information which are so ephemeral seems to be a bit overkill. I don't see a significant benefit of splitting the "active" state into a separate table: at the end of the day, both tables will have to be queried. If a table split were to happen I'd rather split it as a table to store Q2A core settings and a table were plugins store their settings. But that's a different discussion, anyway.

So these approaches seem to be tackling the concurrency issue, which is actually based on the inability to invalidate a cache. Invalidating a cache makes sense and is part of the natural life-cycle of the cache. Considering 1) no action in Q2A does so, 2) the frequency in which this will happen and 3) the amount of cache invalidations that will happen during each of the version checks, I think the cache invalidation is the right way to go.

What are your thoughts on this?

@svivian
Copy link
Collaborator

svivian commented Aug 31, 2020

Wouldn’t that still have the same concurrency problem? If the checks for plugin A and B finish at the same time, even bypassing the cache they will both fetch the same value from the database. Then they will update independently and only one will be saved.

Using promises as a barrier would result in the need to send all the plugin data that needs to be stored in the database to the client.

Only the version numbers. If you have 10 plugins being checked you’ll get back 10 version numbers. Those are then sent to the server in one request and the option is updated. I guess I’m not understanding what you’re saying the problem is.

@pupi1985
Copy link
Contributor Author

Sorry for the long delay here.

Wouldn’t that still have the same concurrency problem?

No. This is because the versions checks are all fired at the same time so the options for each thread are cached at the same time. This results in all the threads having the initial snapshot. Then all threads add their own piece of information to the initial snapshot and save, once the HTTP request is done.

What needs to be done, is just to re-fetch the record from the database before the save, but once the HTTP request is done. This shouldn't generate any concurrency issues (apart from the ones considered acceptable for not using transactions).

Only the version numbers.

You will also need the plugin ID to match the version number to something (if I understand your suggested solution correctly). As I see it, this process would be:

  1. The plugins page is refreshed
  2. The main thread fires all the AJAX requests as promises and waits on all of them to finish
  3. Each AJAX request fetch the version number (by means of the version.php controller) and also provides the plugin ID to the main thread
  4. All AJAX requests finish
  5. The main thread fires one last request to a new controller (e.g. version-update.php) which only purpose will be to act as a facade of the qa_opt() function (or maybe several calls to it)

As I see it, it is a bit over-complicated as all this could be fixed by performing the call to qa_opt() in version.php, fire by each AJAX request. We save one controller to the expense of invalidating a cache. The only drawback I see here is that in my suggested approach there could be multiple write operations. However, considering the frequency of execution, they are negligible.

I actually had my solution 80% developed and went ahead and completed it so that you could take a look at it. I will make some comments in the PR.

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.

2 participants