Fixed Redundant IO check on loading phrase group

[Xon]

Affected version

2.2.5

In \XF\Language::loadPhraseGroup, there are redundant IO/kernel transitions;

$file = $this->groupPath . "/l$this->id/$group.php";
if ($this->groupPath && file_exists($file) && is_readable($file))
{
   $this->phraseCache = array_merge($this->phraseCache, include($file));
   $this->groupsCached[$group] = true;
}
else
{
   $this->groupsCached[$group] = false;
}

This results in the strace;

access("/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php", F_OK) = 0
access("/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php", R_OK) = 0
lstat("/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php", {st_mode=S_IFREG|0644, st_size=177, ...}) = 0
lstat("/var/www/html/internal_data/code_cache/phrase_groups/l0", {st_mode=S_IFDIR|S_ISGID|0775, st_size=12288, ...}) = 0
lstat("/var/www/html/internal_data/code_cache/phrase_groups", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
lstat("/var/www/html/internal_data/code_cache", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
lstat("/var/www/html/internal_data", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
open("/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=177, ...}) = 0
read(3, "<?php\nreturn array (\n  'inline_m"..., 177) = 177
close(3)                             = 0

There are additional kernel transitions over the stats calls required by include. There is a race condition between file_exists and is_readable and the include call, so this can throw anyway.

Something like this, where the file is loaded and then handle if any exception being thrown;

$file = $this->groupPath . "/l$this->id/$group.php";
try
{
  $phrases = include($file);
}
catch (\Throwable $e)
{
  $phrases = false;
}
if (\is_array($phrases))
{
  $this->phraseCache = \array_merge($this->phraseCache, $phrases);
  $this->groupsCached[$group] = true;
}
else
{
  $this->groupsCached[$group] = false;
}

Results in this strace;

lstat("/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php", {st_mode=S_IFREG|0644, st_size=177, ...}) = 0
lstat("/var/www/html/internal_data/code_cache/phrase_groups/l0", {st_mode=S_IFDIR|S_ISGID|0775, st_size=12288, ...}) = 0
lstat("/var/www/html/internal_data/code_cache/phrase_groups", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
lstat("/var/www/html/internal_data/code_cache", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
lstat("/var/www/html/internal_data", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
open("/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=177, ...}) = 0
read(3, "<?php\nreturn array (\n  'inline_m"..., 177) = 177
close(3)                                = 0

Spoiler: scripts used

# cat a.php
<?php

$file = '/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php';
try
{
$a = include($file);
}
catch (Throwable $e)
{
$a = false;
}
var_dump(is_array($a));

# strace php a.php
....

# cat b.php
<?php

$file = '/var/www/html/internal_data/code_cache/phrase_groups/l0/inline_moderation.php';
if (file_exists($file) && is_readable($file))
{
$a = include($file);
}
else
{
$a = false;
}
var_dump(is_array($a));

....

 

[Kirby]

Nice investigation

A similar issue might exist in \XF\Template\Templater::getTemplateDataFromSource()

if (!file_exists($file))
{
    return false;
}

return include($file);

While "blindly" including the file and handling errors afterwards seems slower (in my profiling tests) if there is an error, it is faster in the normal case (eg. when there is no error).

 

[Xon]

Nice investigation

A similar issue might exist in \XF\Template\Templater::getTemplateDataFromSource()

Already created a thread for that bug report; https://xenforo.com/community/threads/redundant-io-check-on-loading-template.195753/

While "blindly" including the file and handling errors afterwards seems slower (in my profiling tests) if there is an error, it is faster in the normal case (eg. when there is no error).

The exception isn't free to generate (which is even more expensive in debug mode), so maybe just make it @include and check the return value on being false if any additional reporting wants to be done would be an idea.

 

[PaulB]

Ideally, include($file) should be changed to include $file or (include $file). include et al have different precedence from a function call, and this code is dangerously close to a precedence-related bug.

Does opcache mitigate the redundant syscalls? If I understand correctly, it should.

 

[XF Bug Bot]

Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.7).

Change log:

Remove redundant file existence check when loading phrase groups

There may be a delay before changes are rolled out to the XenForo Community.