CMS MADE SIMPLE FORGE

Frontend Users

 

[#12276] FEU memory leak / logic-loop ?

avatar
Created By: Phil Scoltock (mantapro)
Date Submitted: Mon Mar 16 22:12:33 -0400 2020

Assigned To: Robert Campbell (calguy1000)
Version: 3.2
CMSMS Version: 2.2.10
Severity: Minor
Resolution: Invalid
State: Open
Summary:
FEU memory leak / logic-loop ?
Detailed Description:
Following on from bug report 12269  I have been developing a simple module to
extend the functionality I need for FEU.

In my module I do the following FEUExtensions.module.php

\CMSMS\Hookmanager::add_hook(
'FrontEndUsers::OnUpdateUser','FEUExtensions::UpdateVerification');
.
.
.
public static function UpdateVerification( $resp ) {
       $feu = \cms_utils::get_module('FrontEndUsers');
        .
        . I check some FEU properties
        . If a combination of properties is OK then I do 

              $t = strval(time());
              $feu->SetUserPropertyFull("verification_date", $t,$resp['id']);
  
I have noticed that the one line "$feu->SetUserPropertyFull("verification_date",
$t,$resp['id']);" crashes Apache in the development/test Xampp environment I as
using. The default values for memory_limit for Xampp run on a Windows PC is a
known problem - so I have increased it a few times - the only difference that
makes is that the it runs for longer and longer before crashing.
  
Even if you are not a fan of what I need to do or my programming to achieve what
is needed - regardless I though you would want to know that there is something
not right with the FEU module (sorry I lack the expertise to diagnose memory
leaks - I have checked the php and apache error logs - empty I think it is
crashing before reporting)

From my diagnostic I can see that the API SetUserPropertyFull is executed
numerous times by FEU and successful every time. But when called from another
module then execution time is goes from milliseconds to minutes


History

Comments
avatar
Date: 2020-03-16 22:23
Posted By: Phil Scoltock (mantapro)

Thinking about it further - I already have another section of code that runs ok
that uses the  $feu->SetUserPropertyFull API triggered via the same hook - that
executes perfectly - it updates a text field

Whereas the possible memory leak / loop issue highlighted above is a DATE field
- and if I prevent it from executing the  UPDATE sql when for the specific
property ID in my test case - it still hangs so I don't think the problem is in
the Database class
      
avatar
Date: 2020-03-17 00:15
Posted By: Phil Scoltock (mantapro)

API $feu->ForceChangeSettings($resp['id'],TRUE); has same problem
      
avatar
Date: 2020-03-21 23:19
Posted By: Robert Campbell (calguy1000)

I think you have a recursion issue or something.  I call SetUserPropertyFull
from many different modules with no difficulty.
      
avatar
Date: 2020-03-24 14:51
Posted By: Phil Scoltock (mantapro)

Yes I too think that the problem may be some form of logic loop or recursive
behaviour but my module is so tiny I doubted it was in there. (as the only thing
I wanted to do is a fragment of code that used to be done via an event) so my
module only has two files: the primary modulename.module.php and the
\lang\en_US.php

I have stripped down my module to the following code:
<?php
final class FEUExtensions extends \CMSModule {

   private static $userVerified;

   public function GetName() { return 'FEUExtensions'; }
   public function GetVersion() { return '0.1.0'; }
   public function MinimumCMSVersion() { return '2.2.10'; }
   public function HasAdmin() { return FALSE; }
   public function IsPluginModule() { return FALSE; }
   public function LazyLoadFrontend() { return FALSE; }
   public function LazyLoadAdmin() { return FALSE; }
   public function GetFriendlyName() { return $this->Lang('friendlyname'); }
public function GetAdminDescription() { return $this->Lang('moddescription');
}
public function GetDependencies() { return
['CGExtensions'=>'1.65','FrontEndUsers'=>'3.0']; }
   public function GetAuthor() { return 'MantaPro - Phil Scoltock'; }
   public function GetAuthorEmail() { return 'philip.scoltock@mantapro.biz'; }
   

   public function InitializeFrontend() {
   
\CMSMS\Hookmanager::add_hook(
'FrontEndUsers::OnUpdateUser','FEUExtensions::UpdateVerification');
       
    }
   
   
   public static function UpdateVerification( $resp ) {
       
       $handle = fopen("track.txt", "a");
       fwrite($handle, time().PHP_EOL);
       fclose($handle);

       $feu = \cms_utils::get_module('FrontEndUsers');
       $t = strval(time());
       // $feu->SetUserPropertyFull("verification_date", $t,$resp['id']);
       
      }
     
}
?>

This is running in a brand-new; out-of-the-box standard CMSMS v2.2.10 install
(with standard installed templates and page content - i.e. ZERO customisation).
Installed FEU v3.2 and its dependancies CGE 1.65.2 and CGSimpleSmarty v2.2.1

Note that in my module the call to feu->SetUserPropertyFull(); is commented out.

When I submit the change settings screen at the front end; the content of
track.txt is:
1585007437
1585007437
1585007437
1585007437
1585007437
1585007438

Six timestamps because I guess the hook is trigger after each DB update for
email; password; the text field; the hidden date field; etc.
But importantly only executed 6 times. (On a side note it is a real shame that
the name of the field being updated isn't part of the hook).

So FEU onUpdateUser hook is triggered once for property being updated to the DB
- all OK so far.

I amended the code for the SetUserPropertyFull method in
\FrontEndUsers\lib\class.FrontEndUsersManipulator.php to include corresponding
append-to-file code

public function SetUserPropertyFull(string $title,string $data,int $userid) :
bool
    {
        $title = trim($title);
if( !$title || $userid < 1) throw new \InvalidArgumentException('Invalid
uid passed to '.__METHOD__);
        $defn = $this->GetPropertyDefn($title);
        if( !$defn ) return FALSE;
if( $defn['force_unique'] &&
!$this->IsUserPropertyValueUnique($userid,$title,$data) ) return FALSE;
        
       $handle = fopen("trackFEU.txt", "a");
       fwrite($handle, time().PHP_EOL);
       fclose($handle);
        
                $db = $this->GetDB();
$q = "SELECT * FROM ".CMS_DB_PREFIX."module_feusers_properties WHERE
title=? AND userid=?";
        $r = $db->Execute($q, [$title, $userid]);

        if (!$r || ($r->NumRows()==0)) {
$q="INSERT INTO ".CMS_DB_PREFIX."module_feusers_properties
(userid,title,data) VALUES (?,?,?)";
            $r=$db->Execute($q, [$userid, $title, $data]);
        } else {
            $row=$r->FetchRow();
$q="UPDATE ".CMS_DB_PREFIX."module_feusers_properties SET data=?
WHERE id=?";
            $r=$db->Execute($q, [$data, (int)$row['id']]);
        }

        return ($r!=false);
    }
  
And I uncommented the SetUserPropertyFull in my module

Now when run (remembering it is a brand standard CMSMS install) Apache
eventually crashes.
I won't copy the contents of trackFEU.txt here - there was far too much of it -
4199 lines of  the timestamp!!!.

Interestingly there is an identical number of lines in the track.txt file being
written by my module i.e. core CMSMS is doing the InitializeFrontend() method
4199 times.

So the recursion isn't simply a logic-loop in FEU rather on this evidence then
FEU probably has a loop with a redirect or loop in the core ?
I've used the same SetUserPropertyFull method to get and set other text (i.e.
not a date) properties triggered by the exact same hook; my suspision therefore
is a redirect somewhere in the logic associated with the $this->GetPropertyDefn
or $this->IsUserPropertyValueUnique methods when the property type is DATE. But
my debugging has not got that far yet. There must be some better debug tools
than writing to a file and PHP echos?!.

I can try installing a new CMSMS install on one of the Unix servers at my 1&1
Ionos account; perhaps on a more powerful webserver it wont crash but it
shouldn't be looping 1000s of times. And on a shared production server at 1&1
they do not provide access to the error logs etc; so Xampp is a more informative
development environment.

The wondered if having HasAdmin() & IsPluginModule() both set to FALSE might be
unexpected combination casing problems, but set to TRUE Apache still crashes
after looping 1000s of times and these both set to False is an accurate status
for a snippet of logic in a mini-module that extends another module via a hook
(i.e. replaces an Event+UDT)
      
avatar
Date: 2020-03-26 19:47
Posted By: Phil Scoltock (mantapro)

Tried it on a production 1&1 Ionos Unix Server. Pretty much similar result. 

When doing Change Settings in front end it aborts with msg of

Fatal error: Allowed memory size of 33554432 bytes exhausted (tried to allocate
20480 bytes) in
/homepages/38/d242029264/htdocs/cmsms/lib/classes/Database/class.Connection.php
on line 298

Looking at the 2 .txt files written (as described above) they are both > 3k
entries so something in FEU/CGE is looping and eating up memory
      
avatar
Date: 2020-10-20 17:37
Posted By: Robert Campbell (calguy1000)

the FrontEndUers::OnUpdateUser is called from SetUserPropertyFull so your code
would cause an infinite loop.

You want to use 

FrontEndUsers::BeforeChangeSettings  or 
FrontEndUsers::ChangeSettingsAfterValidate
      
Updates

Updated: 2020-10-20 17:37
resolution_id: 5 => 9

Updated: 2020-03-16 22:23
resolution_id: => 5