@mattab opened this Issue on June 8th 2012 Owner

In Truncate.php#L42, Truncate is applied recursively.

When expanded = 0, sub-datatables are not available in Piwik_DataTable_Manager->tables.

In that case, Truncate filter should not try to load sub-datatables as in Truncate.php#L43 :

Piwik_DataTable_Manager::getInstance()->getTable($idSubTable);

It works 'fine' when $this->c[corresponds to an empty entry in [https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables](self::DATATABLE_ASSOCIATED]) because an exception is raised in Manager.php#L75 and silenced in Truncate.php#L45.

However, in some random but legitimate cases, $this->c[does correspond to a non-empty entry in https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables. However, this non-empty entry has nothing to do with the sub-datable as they are not loaded in memory.

The worst case scenario, as happening for users generating reports, see forum post, is when _$this->c[self::DATATABLEASSOCIATED] of a Piwik_DataTable_Row is equal to its enclosing Piwik_DataTable->currendId. This is when the infinite recursion happens.

All of this is fine, when expanded = 1 because sub-datatable are loaded in memory and their IDs ie. $this->c[are synchronized with [https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables](self::DATATABLE_ASSOCIATED]), see Piwik_Archive_Single L355 :

// and update the subtableID so that it matches the newly instantiated table 

How to fix ?

Truncate filter should not perform recursive filtering when sub-datatables are not requested (ie. expended = 0)

Truncate filter should use Piwik_DataTable_Filter->filterSubTable which checks if filters should be applied recursively :

71          if(!$this->enableRecursive)
72          {
73              return;
74          }
@mattab commented on June 21st 2012 Owner

Reported by many users in http://forum.piwik.org/read.php?2,90170 as well

@julienmoumne commented on June 26th 2012 Member

As described in comment:1:ticket:3191, one issue we found is infinite nesting calls, see http://pastebin.com/Kdrkb7V9

Applying the following patch http://forum.piwik.org/read.php?2,90170,page=2#msg-90681 solves the infinite nesting.

However, we have yet to know :

  • how a reflective cycle can happen on a DataTable
  • how we should fix this
  • if this issue is the same for all users reporting issues with scheduled reports, e.g: #3254
@mattab commented on June 27th 2012 Owner

Thank you Julien for the research and follow up on the forum!!!
Great stuff :)

There is indeed a deeper bug that we need to somehow find out: how can it happen that we end up with a Subtable with the same ID as the parent table??!
This should not be possible. Maybe we need to prevent this at the datatable level somehow.

If we don't fix the root issue, we will have to patch as you did all other filters & codes that use subtables the same way...

It would be very good if we could understand how such buggy data happens & replicate the bug... I asked in the thread

@mattab commented on July 27th 2012 Owner

Thank you Julien for this great bug hunt and kuddos on finding the prize... I agree with your clear description & the suggestion for a fix.

Did you quickly check that all filters are "safe" with this regards?

@julienmoumne commented on July 29th 2012 Member

Did you quickly check that all filters are "safe" with this regards?

Running a search on 'getIdSubDataTable' in directory 'core\DataTable\Filter' yields two results :

  • Truncate.php
  • PatternRecursive.php

The latter is a recursive version of Pattern.php.

I am wondering if PatternRecursive.php was necessary. Pattern.php could maybe be rewritten to support recursion using Piwik_DataTable_Filter->filterSubTable.

Anyway, PatternRecursive.php should only be used when dealing with expanded results. Meaning the issue we have with Truncate.php should not occur with this filter.

@mattab commented on July 30th 2012 Owner

Thanks for confirming! Yes PatternRecursive should be Pattern and is not necessary indeed.

@mattab commented on July 30th 2012 Owner

(In [6593]) Refs #3207 - Fixing (?) a 4 year old issue with the DataTable_Manager:

  • Now a DataTable_Row knows if the sub-datatable was loaded in memory, which is useful when __destruct() is called to free memory

I think the ticket is closed but pending my code being reviewed
Also, we could still get rid of PatternRecursive

@mattab commented on July 30th 2012 Owner

(In [6594]) Refs #3207 - Fixing regressions

@mattab commented on July 30th 2012 Owner

The build is failing but all_tests pass on my box! I'll look into it tomorrow unless you find the bug :-)

@mattab commented on July 31st 2012 Owner

(In [6598]) Refs #3207 ensure that __destruct() is called on the subtables

@mattab commented on July 31st 2012 Owner

(In [6606]) Refs #3084 This should fix it! but I'll wait for @owen confirmation,
Refs #3207 Now calling destroy() prior to setTableDeleted as expected

@mattab commented on July 31st 2012 Owner

(In [6607]) Refs #3207 the "hack" in DataTable_Row to set tables loaded in memory with a negative ID implies that the table IDs are != 0 --- thanks Julien for the review and good catch

@mattab commented on July 31st 2012 Owner

It looks like it's now fixed. Please, reopen or comment if you find any problem or suggestion!

@julienmoumne commented on August 2nd 2012 Member

(In [6654]) refs #3207 unit tests

This Issue was closed on August 15th 2012
Powered by GitHub Issue Mirror