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

Multiple calls of 'include' do not check for file modification HHVM 3.5.0 #4797

Closed
uwetews opened this Issue Feb 7, 2015 · 8 comments

Comments

Projects
None yet
6 participants
@uwetews

uwetews commented Feb 7, 2015

No description provided.

@fredemmott

This comment has been minimized.

Show comment
Hide comment
@fredemmott

fredemmott Feb 9, 2015

Contributor

This is not currently supported. While we do intend to fix this eventually when running from source files, keep in mind it will not be fully compatible, as we will not support this (or any kind of eval()/generated code) in Repo Authoritative mode.

One approach to work around this for non-repo-authoritative mode is to put a timestamp (if it's updated rarely) or hash of the source in the filename.

Contributor

fredemmott commented Feb 9, 2015

This is not currently supported. While we do intend to fix this eventually when running from source files, keep in mind it will not be fully compatible, as we will not support this (or any kind of eval()/generated code) in Repo Authoritative mode.

One approach to work around this for non-repo-authoritative mode is to put a timestamp (if it's updated rarely) or hash of the source in the filename.

@fredemmott

This comment has been minimized.

Show comment
Hide comment
@fredemmott

fredemmott Feb 9, 2015

Contributor

There is an incomplete patch at #2357

This is also related to #1447

Contributor

fredemmott commented Feb 9, 2015

There is an incomplete patch at #2357

This is also related to #1447

@uwetews

This comment has been minimized.

Show comment
Hide comment
@uwetews

uwetews Feb 9, 2015

Smarty does compile template source into PHP files and/or creates output PHP cache files.
When a page is called normally the compiled or cache template PHP files are included for processing.
These files contain code and data about template and subtemplate source dependicies and other information to determine if the compiled template or cache file is still valid or needs to be updated.
In the later case Smarty does overwrite the include file with same name but new content. The new include file is loaded again for processing and displaying the page.

Because of the problem the include file was updated on disk, but the old content will be displayed on the current request. The new content will be displayed correct on the next request for that page.

PHP does recheck the filestats on each include/require, will detect the file modification and include the updated file when called multiple times in the same script.

Using timestamp or hash of source does not work in our case because compiled and cache files may contain content of many individual template source files. So the file name can't be computed when a page is called.

I think this could be a major problem for all applications which will create/update PHP or Hack files.

Currently many of our units test do fail under HHVM as we do test recompilation and cache updates very intense. Currently we do have about 1900 individual tests to run. I tried to isolate the critical tests and run them in separate processes. But it seems that there could be race conditions between the individual processes as I get random errors.

uwetews commented Feb 9, 2015

Smarty does compile template source into PHP files and/or creates output PHP cache files.
When a page is called normally the compiled or cache template PHP files are included for processing.
These files contain code and data about template and subtemplate source dependicies and other information to determine if the compiled template or cache file is still valid or needs to be updated.
In the later case Smarty does overwrite the include file with same name but new content. The new include file is loaded again for processing and displaying the page.

Because of the problem the include file was updated on disk, but the old content will be displayed on the current request. The new content will be displayed correct on the next request for that page.

PHP does recheck the filestats on each include/require, will detect the file modification and include the updated file when called multiple times in the same script.

Using timestamp or hash of source does not work in our case because compiled and cache files may contain content of many individual template source files. So the file name can't be computed when a page is called.

I think this could be a major problem for all applications which will create/update PHP or Hack files.

Currently many of our units test do fail under HHVM as we do test recompilation and cache updates very intense. Currently we do have about 1900 individual tests to run. I tried to isolate the critical tests and run them in separate processes. But it seems that there could be race conditions between the individual processes as I get random errors.

@r3wt

This comment has been minimized.

Show comment
Hide comment
@r3wt

r3wt Feb 9, 2015

i just disable the templating and use nginx fastcgi cache as a work around.

r3wt commented Feb 9, 2015

i just disable the templating and use nginx fastcgi cache as a work around.

@mikehaertl

This comment has been minimized.

Show comment
Hide comment
@mikehaertl

mikehaertl Mar 14, 2015

+1 For fixing this. Another affected package is the yii2-composer package, which is a requirement for installing Yii2 via composer. This is a composer plugin that updates a list of installed extensions in vendor/yiisoft/extensions.php and therefore reads and writes that file a couple of times during the runtime of composer.

mikehaertl commented Mar 14, 2015

+1 For fixing this. Another affected package is the yii2-composer package, which is a requirement for installing Yii2 via composer. This is a composer plugin that updates a list of installed extensions in vendor/yiisoft/extensions.php and therefore reads and writes that file a couple of times during the runtime of composer.

uwetews referenced this issue in smarty-php/smarty Apr 18, 2015

@belgattitude

This comment has been minimized.

Show comment
Hide comment
@belgattitude

belgattitude May 13, 2015

+1 for fixing include behaviour (a way to include multiple time a same file). I ran into an issue with some unit tests independently than smarty.

belgattitude commented May 13, 2015

+1 for fixing include behaviour (a way to include multiple time a same file). I ran into an issue with some unit tests independently than smarty.

@uwetews

This comment has been minimized.

Show comment
Hide comment
@uwetews

uwetews Jul 26, 2015

For Smarty I have implemented a workaround for this problem.
The patch is the Smarty dev-master branch and will later be included in Smarty 3.1.28.

uwetews commented Jul 26, 2015

For Smarty I have implemented a workaround for this problem.
The patch is the Smarty dev-master branch and will later be included in Smarty 3.1.28.

crodas added a commit to crodas/FileUtils that referenced this issue Jan 5, 2016

Making HHVM friendly
1. Added File::writeAndInclude which solves facebook/hhvm#4797
2. Improved Cache internal includes to be HHVM friendly
3. Added HHVM to the tests

archibaldhaddock pushed a commit to archibaldhaddock/hhvm that referenced this issue Feb 22, 2016

hhvm-bot pushed a commit that referenced this issue Oct 6, 2016

gwen Hhvm Bot
correcting issue #4797
Summary:
I hope this time everything is fine...
Closes #6854

Differential Revision: D2961936

Pulled By: aorenste

fbshipit-source-id: 5e8db79d67447a14f9914cb2b1ccee1aeb16b7f5

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-MediaWikiFarm that referenced this issue Dec 5, 2016

Improve testing infrastructure
* Instead of a hacky 'sed -i' in composer.json, dynamically declare the class
  MediaWikiTestCase if missing (when PHPUnit is called directly).
* Check if phpdbg is installed, else use php
* Skip two tests when running HHVM because of issue #4797
  "Multiple calls of 'include' do not check for file modification HHVM 3.5.0"
  facebook/hhvm#4797
  Possibly a workaround will be later added, but for now try to render
  MediaWikiFarm compatible with WMF CI suites
* Fixed two phpcs issues
* Dynamically create two JSON files with bad syntax (used in a test) to avoid
  trigerring a CI error report
* Made phpdoc optional for now

Bug: T151879
Change-Id: I17604c11e773f74f2c8b7ccb6aa7919cb729d552

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this issue Dec 5, 2016

Updated git submodules
Project: mediawiki/extensions/MediaWikiFarm master 74cef8cbb8dea604bf6be4d41c81dc2cfa608e53

Improve testing infrastructure

* Instead of a hacky 'sed -i' in composer.json, dynamically declare the class
  MediaWikiTestCase if missing (when PHPUnit is called directly).
* Check if phpdbg is installed, else use php
* Skip two tests when running HHVM because of issue #4797
  "Multiple calls of 'include' do not check for file modification HHVM 3.5.0"
  facebook/hhvm#4797
  Possibly a workaround will be later added, but for now try to render
  MediaWikiFarm compatible with WMF CI suites
* Fixed two phpcs issues
* Dynamically create two JSON files with bad syntax (used in a test) to avoid
  trigerring a CI error report
* Made phpdoc optional for now

Bug: T151879
Change-Id: I17604c11e773f74f2c8b7ccb6aa7919cb729d552
@mofarrell

This comment has been minimized.

Show comment
Hide comment
@mofarrell

mofarrell Mar 24, 2017

Contributor

This was fixed.

Contributor

mofarrell commented Mar 24, 2017

This was fixed.

@mofarrell mofarrell closed this Mar 24, 2017

hhvm-bot added a commit that referenced this issue Mar 28, 2017

Fixed require_once to only include file once even if the file has alr…
…eady been modified.

Summary:
This got introduced when fixing #4797

Closes #7722

Reviewed By: markw65

Differential Revision: D4779333

fbshipit-source-id: ab3bbc998812a7e75885ca5faed5824002724f67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment