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

Avoid error messages for restricted opcache API #8188

Merged
merged 1 commit into from
Jul 20, 2018

Conversation

arnowelzel
Copy link
Contributor

As mentioned in #3602 the server log gets filled with messages like this, when opcache_invalidate() or opcache_reset() can not be called due to a restricted API:

Zend OPcache can't be temporary enabled (it may be only disabled till the end of request) at Unknown#0

It makes to report errors in the log for these calls as the cause for it usually can not be changed easily. Also note that other cache calls for APC etc. are also used with a preceding @ sign to avoid error messages.

Having dozens or hundreds of error messages without indicating the reason for the error does not help - a better approach to deal with this and to let the administrator know that there may be a problem would be to check the call in the security admin tests and show the result there with a message like "it seems, opcache can not be cleared if needed - please be aware, that changes in the configuration might not take place immediately".

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Mmm I'm not sure about this. I get that the warnings make no sense if you can't change it. But maybe we should handle this differently... as we do expect those functions to work...

@nickvergessen
Copy link
Member

I agree, don't think it's good to hide this. This will result in wrong stuff being retained in the cache, so yeah you need to take actions to fix it.

@arnowelzel
Copy link
Contributor Author

arnowelzel commented Feb 9, 2018

In my case it is not possible at all to allow PHP scripts to invalidate the opcache on their own for security reasons. When I looked up the problem in the first place I found workarounds like to allow NextCloud as the one and only global cache management in php-fpm etc.. - which is also not an option for me. So I ended up with a ever growing log filled with opcache related errors.

That's why I suggested to add a warning in the admin page to let the admin know that there may be a problem - similar to the current message which informs you about a missing opcache.

A even better approach would be not to rely on a properly cached config.php at all. Use a file which is actively read and not some code which is included. Yes, this might cause an additional file access which may slow down things. But in fact a small configuration file with just a few KB of data which is read all the time will be cached by the operating system anyway and I doubt that this will cause any significant bigger load.

Edit: and generally - if opcache or any other cache causes problems with a PHP application, the cache configuration should be fixed (e.g. make sure that opcache.validate_timestamps is enabled and opcache.revalidate_freq is set to a sensible value). Requiring the ability to flush cache using opcache_invalidate() to ensure correct application behaviour looks a bit broken to me. So it should be completely sufficient to check the configuration and warn the admin if anything looks bad.

BTW: Why was cache invalidation introduced in the first place at all?

@MorrisJobke
Copy link
Member

That's why I suggested to add a warning in the admin page to let the admin know that there may be a problem - similar to the current message which informs you about a missing opcache.

But a missing opcache is fine and only has performance issues - a not reset opcache maybe causes issues due to the old content.

Edit: and generally - if opcache or any other cache causes problems with a PHP application, the cache configuration should be fixed (e.g. make sure that opcache.validate_timestamps is enabled and opcache.revalidate_freq is set to a sensible value). Requiring the ability to flush cache using opcache_invalidate() to ensure correct application behaviour looks a bit broken to me. So it should be completely sufficient to check the configuration and warn the admin if anything looks bad.

Correct ... we should think about this, but hiding it is definitely not the way to go IMO.

@arnowelzel
Copy link
Contributor Author

arnowelzel commented Apr 11, 2018

But a missing opcache is fine and only has performance issues - a not reset opcache maybe causes issues due to the old content.

But this is a problem of the runtime environment and not Nextcloud.

Maybe I did not make this clear enough: Usually it is never neccessary to reset the opcache within a PHP script at all. Whenever a script changes, the opcache will be updated anyway. If this is not the case, then the server has to be configured properly and the solution should not be to rely on opcache_reset() or opcache_invalidate() to deal with a defect server configuration.

Also see: http://php.net/manual/en/opcache.configuration.php

Please note that timestamp checking is the default and PHP will check the cache with every request by default. If a user changes this, it is their problem and not the problem of Nextcloud.

And as I also already mentioned: in my case Nextcloud can't call opcache_reset() or opcache_invalidate() but the OPcache works completely fine as well. There is no problem! So why the log entries? A warning in the admin section is totally sufficient.

BTW: Can you refer to any issue in the past which was caused by an old cached copy of a file which could only be solved by using opcache_reset() or opcache_invalidate() and not fixing the server configuration?

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

It kind of makes sense to ignore it. It is anyways only a failsafe fallback.

@francoism90
Copy link

Status update? I'm also having this issue and as @arnowelzel described it shouldn't be needed anyway to call those methods.

@MorrisJobke MorrisJobke added the stale Ticket or PR with no recent activity label Jun 19, 2018
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🐘

@MorrisJobke MorrisJobke merged commit e0d9c43 into nextcloud:master Jul 20, 2018
@nextcloud-bot nextcloud-bot removed the stale Ticket or PR with no recent activity label Jul 20, 2018
joshtrichards added a commit that referenced this pull request Aug 28, 2024
Make changes recently added via #44230 match #8188 to avoid failures in restricted hosting environments.

Fixes #47562

Signed-off-by: Josh <josh.t.richards@gmail.com>
backportbot bot pushed a commit that referenced this pull request Aug 30, 2024
Make changes recently added via #44230 match #8188 to avoid failures in restricted hosting environments.

Fixes #47562

Signed-off-by: Josh <josh.t.richards@gmail.com>
backportbot bot pushed a commit that referenced this pull request Aug 30, 2024
Make changes recently added via #44230 match #8188 to avoid failures in restricted hosting environments.

Fixes #47562

Signed-off-by: Josh <josh.t.richards@gmail.com>
backportbot bot pushed a commit that referenced this pull request Aug 30, 2024
Make changes recently added via #44230 match #8188 to avoid failures in restricted hosting environments.

Fixes #47562

Signed-off-by: Josh <josh.t.richards@gmail.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants