Use file_get_contents instead of SplFileObject::fpassthru() #6224

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@armetiz

Hi there.
For a local operation, I'm parsing some Apache Log Files.
It's a simple code using the Finder, the Symfony\Finder\SplFileInfo::getContents function & preg_match_all function : Sample

The biggest file size is 500M, and I have around ten files.
Without the PR, I have to set the "memory_limit" value to 1G to use the above script. And with my PR the "memory_limit" is really near the biggest file, it's working with memory_limit set to 550M.

Also,
When the PHP Fatal error: Allowed memory size comes, without the PR, the standard output is spammed by the loaded content.. Which is really annoying.

I'm using PHP 5.3.17 on a MacOS.

@fabpot
Symfony member

The usage of fpassthru was introduced in #4751... but the reason is not really clear to me anymore. ping @gajdaw, @stealth35

@gajdaw

The reason for fpassthru() was to use internal exceptions handling provided by SplFileObject.

I didn't compare fpassthru() to file_get_contents(). I assumed that they both work in the same way.

If file_get_contents() is better, then I think the best solution is to revert changes introduced in PR:

https://github.com/symfony/symfony/pull/4751/files

@fabpot fabpot added a commit that closed this pull request Dec 10, 2012
@fabpot fabpot Revert "merged branch gajdaw/finder_splfileinfo_fpassthu (PR #4751)" …
…(closes #6224)

This reverts commit 5608c0c, reversing
changes made to 38c30b7.

Conflicts:
	src/Symfony/Component/Finder/SplFileInfo.php
a1734dd
@fabpot fabpot closed this in a1734dd Dec 10, 2012
@armetiz

An other problem appears.

If I open a big file (e.g 500M) with a memory_limit of 300M, the process shutdown without any error and any other information...

    public function getContents()
    {
        $level = error_reporting(0);
        $content = file_get_contents($this->getRealpath());
        error_reporting($level);
        if (false === $content) {
            $error = error_get_last();
            throw new \RuntimeException($error['message']);
        }

        return $content;
    }

The normal process of the getContents function is to throw a RuntimeException when there is a problem on file_get_contents.. Which is not the case when a Fatal Error occurs...

Maybe we could add a register through : register_shutdown_function and check the error_get_last function.. But there is no unregister_shutdown_function .. Any idea ?

I'm running a CLI script with PHP 5.3.17.

@stof
Symfony member

@armetiz catching the fatal error in a register_shutdown_function will not allow you to handle the error inside this function as the shutdown is too late. Other part of your app may have been destructed already.
And if it is only about providing a better error page for developers, this is done in 2.2

@armetiz

@stof, yes you right about the register_shutdown_function

Like I said, I'm using a short script via CLI. I'm using the Finder as a standalone component.

ini_set('memory_limit', '100M');

use Symfony\Component\Finder\Finder;

$finder = new Finder();
$finder->files()
        ->in(__DIR__ . "/../")
        ->name("access_log*");

echo "begin\n";

foreach ($finder as $file) {
        $contents = $file->getContents();

        echo count($contents) . "\n";

        unset($contents);
}

echo "end\n";

If I'm running this script with some files sized up to 200M, the process will display "begin" and that's all.. No error.

IMHO, what I want to say is that disable error reporting, when there is no error manager is not a good thing. If I remove from my example the three "echo", I can think that it's working like a charm, and it's not.

Regards,
Armetiz.

@mvrhov

You could handle OOM case yourself, by not opening files larger than memory_limit - memory_get_usage()

@armetiz

@mvrhov of course I can. I'm using a script which is working very well, there is no problem for me.

My last comment is just to improve the Finder component.. Because One day, someone will use the Finder with a file bigger than the memory_limit and there will have no error message.

Thomas.

@fabpot fabpot added a commit that referenced this pull request Dec 11, 2012
@fabpot fabpot Merge branch '2.1'
* 2.1:
  fixed CS
  fixed CS
  [Security] fixed path info encoding (closes #6040, closes #5695)
  [HttpFoundation] added some tests for the previous merge and removed dead code (closes #6037)
  Improved Cache-Control header when no-cache is sent
  removed unneeded comment
  Fix to allow null values in labels array
  fix date in changelog
  removed the Travis icon (as this is not stable enough -- many false positive, closes #6186)
  Revert "merged branch gajdaw/finder_splfileinfo_fpassthu (PR #4751)" (closes #6224)
  Fixed a typo
  Fixed: HeaderBag::parseCacheControl() not parsing quoted zero correctly
  [Form] Fix const inside an anonymous function
  [Config] Loader::import must return imported data
  [DoctrineBridge] Fixed caching in DoctrineType when "choices" or "preferred_choices" is passed
  [Form] Fixed the default value of "format" in DateType to DateType::DEFAULT_FORMAT if "widget" is not "single_text"
  [HttpFoundation] fixed a small regression

Conflicts:
	src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php
3c010db
@mmucklo mmucklo pushed a commit that referenced this pull request May 23, 2013
@fabpot fabpot Revert "merged branch gajdaw/finder_splfileinfo_fpassthu (PR #4751)" …
…(closes #6224)

This reverts commit 5608c0c, reversing
changes made to 38c30b7.

Conflicts:
	src/Symfony/Component/Finder/SplFileInfo.php
aa5b8ec
@mmucklo mmucklo pushed a commit that referenced this pull request May 23, 2013
@fabpot fabpot Merge branch '2.1'
* 2.1:
  fixed CS
  fixed CS
  [Security] fixed path info encoding (closes #6040, closes #5695)
  [HttpFoundation] added some tests for the previous merge and removed dead code (closes #6037)
  Improved Cache-Control header when no-cache is sent
  removed unneeded comment
  Fix to allow null values in labels array
  fix date in changelog
  removed the Travis icon (as this is not stable enough -- many false positive, closes #6186)
  Revert "merged branch gajdaw/finder_splfileinfo_fpassthu (PR #4751)" (closes #6224)
  Fixed a typo
  Fixed: HeaderBag::parseCacheControl() not parsing quoted zero correctly
  [Form] Fix const inside an anonymous function
  [Config] Loader::import must return imported data
  [DoctrineBridge] Fixed caching in DoctrineType when "choices" or "preferred_choices" is passed
  [Form] Fixed the default value of "format" in DateType to DateType::DEFAULT_FORMAT if "widget" is not "single_text"
  [HttpFoundation] fixed a small regression

Conflicts:
	src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php
278e3ee
@hackzilla hackzilla pushed a commit that referenced this pull request Dec 10, 2014
@fabpot fabpot Merge branch '2.1'
* 2.1:
  fixed CS
  fixed CS
  [Security] fixed path info encoding (closes #6040, closes #5695)
  [HttpFoundation] added some tests for the previous merge and removed dead code (closes #6037)
  Improved Cache-Control header when no-cache is sent
  removed unneeded comment
  Fix to allow null values in labels array
  fix date in changelog
  removed the Travis icon (as this is not stable enough -- many false positive, closes #6186)
  Revert "merged branch gajdaw/finder_splfileinfo_fpassthu (PR #4751)" (closes #6224)
  Fixed a typo
  Fixed: HeaderBag::parseCacheControl() not parsing quoted zero correctly
  [Form] Fix const inside an anonymous function
  [Config] Loader::import must return imported data
  [DoctrineBridge] Fixed caching in DoctrineType when "choices" or "preferred_choices" is passed
  [Form] Fixed the default value of "format" in DateType to DateType::DEFAULT_FORMAT if "widget" is not "single_text"
  [HttpFoundation] fixed a small regression

Conflicts:
	src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php
70e366a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment