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

Fall back to NullCache on CLI calls if configured cache is not available #27608

Open
MichaIng opened this issue Jun 22, 2021 · 4 comments · May be fixed by #46151
Open

Fall back to NullCache on CLI calls if configured cache is not available #27608

MichaIng opened this issue Jun 22, 2021 · 4 comments · May be fixed by #46151
Assignees
Labels
3. to review Waiting for reviews enhancement feature: caching Related to our caching system: scssCacher, jsCombiner... performance 🚀
Milestone

Comments

@MichaIng
Copy link
Member

MichaIng commented Jun 22, 2021

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Is your feature request related to a problem? Please describe.
After #25440, Nextcloud requires the configured memory cache (config.php > memcache.local) to be available as well for CLI calls. By default PHP APCu is disabled for CLI calls, which already did produce a log entry in the past and it was always recommended to enable it (apc.enable_cli=1) to avoid potential issues with some background jobs. It is however not optimal to initiate a caching instance for a CLI command, which usually does not repeatedly read the same resource, so that building the cache slows down the whole call more than the benefit of having involved resources cached. Also this can lead to a significant increase of memory usage, when CLI calls stack, each with an own cache instance. This matter has been further discussed in #25770.

Describe the solution you'd like
In case of CLI falls, if the configured memory cache is not available (apc.enable_cli=0 and alike), fallback to NullCache, as if no cache was configured (memcache.local unset or none).

Describe alternatives you've considered
An alternative would be to use NullCache in the first place on all CLI calls, ignoring the memcache.local value. But it won't have any benefit as long as the backend itself (PHP module or server) has been initialised or is running anyway.

Additional context
For me, the related code block already looks like it does what I suggest, but obviously it does not, as otherwise the loop, worked around with #25770 wouldn't exist. Here is how I understand the code (obviously wrong), and hopefully someone can point me to the thing I misinterpret:

@MichaIng MichaIng added enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap feature: caching Related to our caching system: scssCacher, jsCombiner... labels Jun 22, 2021
@nickvergessen
Copy link
Member

Ref #25770 (comment)

So the logger can pull in another cache factory but it won't enter this condition again (class exists and isAvailable() always returns true for NullCache), breaking the loop?

While that is true, it does not fully work as per #25770 (comment)

If you do so please have a quick thought how from within a CLI/OCC/cron call a $cache->clear(); (and also remove at least) can be relayed so that APCu for non-CLI looses it's current cached values. If that is not intended we need to rethink the implementation of the cache system even deeper and maybe split in things that can be APCu cached and things that can not.

@MichaIng
Copy link
Member Author

MichaIng commented Jun 23, 2021

To sum up things here:

  • It's the logger call to print the info about non-available backing backend, which causes the loop, which is currently (before #25770) done before NullCache is applied. So the idea was to simply swap it so that NullCache is applied first and the log done afterwards:
		if (!class_exists($localCacheClass) || !$localCacheClass::isAvailable()) {
			if (\OC::$CLI && !defined('PHPUNIT_RUN')) {
				// CLI should not hard-fail on broken memcache but fallback to NullCache
				$localCacheClass = self::NULL_CACHE;
				$this->logger->info($missingCacheMessage, [
					'class' => $localCacheClass,
					'use' => 'local',
					'app' => 'cli'
				]);

it does not fully work as per

If I'm not mistaken, there is no direct way that a CLI call can interact with the cache of running PHP instance, and to me this looks perfectly as it should be, for all privacy and security reasons. An indirect way would be a server-side API, so that the CLI call sends a request to the running Nextcloud instance which then clears the cache internally, or a flag file/config.php setting to trigger it. But note that this did never work before (for PHP-internal cache backends, including also OPcache) and I think it is not something that should be required, respectively where Nextcloud should depend on, else we might open a can of worms, don't we?

If that is not intended we need to rethink the implementation of the cache system

Do you have a specific case in mind where this was ever intended/required? It might be nice for convenience if an external Nextcloud or app upgrade would invalidate related caches, but that's a known limitation depending on TTL and I at least never faced an issue while intensively using the CLI. And keep in mind that APCu is THE recommended caching backend and the majority of Nextcloud users will use it, without anyone ever having the ability to use or invalidate the running PHP instance's cache from CLI. We would have recognised if this did cause any issues, that's for sure.


Regarding the actual purpose of the issue, let's concentrate on whether the above suggestion would make things better or worse compared to how it is done now (after #25770) and handle the CLI cache invalidation topic separately:

Now:

  • When APCu is enabled for CLI, each CLI call will build its own APCu cache and destroy it on exit, without touching the running PHP instance cache.
  • When APCu is disabled for CLI, CLI is disabled.
  • When Redis is used as caching backend, CLI accesses the same cache and can hence also invalidate it.
  • If I'm not mistaken ArrayCache runs in the PHP context like APCu, while being always available for CLI, so similarly the CLI call will build its own cache and is not able to invalidate the running PHP instances cache.

With above suggestion:

  • When APCu is enabled for CLI, nothing changes.
  • When APCu is disabled for CLI, CLI still works but does not build an own APCu cache, which is preferred performance-wise in most cases, as discussed in various related issues and #25770.
  • When Redis, memcached or ArrayCache are configured, again nothing changes.

So comparing the cases, the above suggestion has no downside but preserves the Nextcloud pre-20 ability of using CLI even if the configured caching backend is not available. Furthermore it skips building/allocating the cache then and should hence usually be faster and consume less resources.


A consequent alternative would be btw to always use NullCache for CLI calls, even if the configured caching backend is available. That would make things more predictable, any log could be skipped etc.

@szaimen
Copy link
Contributor

szaimen commented Aug 8, 2021

cc @nextcloud/server for more input on this

@ghost ghost added the stale Ticket or PR with no recent activity label Sep 7, 2021
@nextcloud nextcloud deleted a comment Sep 7, 2021
@ghost ghost removed the stale Ticket or PR with no recent activity label Sep 7, 2021
@ghost ghost added the stale Ticket or PR with no recent activity label Nov 5, 2021
@nextcloud nextcloud deleted a comment Nov 5, 2021
@ghost ghost removed the stale Ticket or PR with no recent activity label Nov 5, 2021
@ghost ghost added the stale Ticket or PR with no recent activity label Dec 5, 2021
@nextcloud nextcloud deleted a comment Dec 5, 2021
@ghost ghost removed the stale Ticket or PR with no recent activity label Dec 5, 2021
@ghost ghost added the stale Ticket or PR with no recent activity label Jan 4, 2022
@nextcloud nextcloud deleted a comment Jan 4, 2022
@ghost ghost removed the stale Ticket or PR with no recent activity label Jan 4, 2022
@ghost ghost added the stale Ticket or PR with no recent activity label Feb 3, 2022
@nextcloud nextcloud deleted a comment Feb 3, 2022
@ghost ghost removed the stale Ticket or PR with no recent activity label Feb 3, 2022
@MichaIng MichaIng self-assigned this Feb 3, 2022
@MichaIng MichaIng added 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Feb 3, 2022
@szaimen szaimen removed the needs info label Mar 6, 2023
@MichaIng MichaIng linked a pull request Jun 26, 2024 that will close this issue
5 tasks
@MichaIng
Copy link
Member Author

It took only 3 years, but here is a PR to re-implement the NullCache fallback for non-PHPUnit CLI calls: #46151

@MichaIng MichaIng added this to the Nextcloud 30 milestone Jun 26, 2024
@MichaIng MichaIng added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Jun 26, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: caching Related to our caching system: scssCacher, jsCombiner... performance 🚀
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants