Memory leak in Shared.DC.ZRDB.Results.SQLAlias

[Michael Brunnbauer]

A leak happens with objects of class Shared.DC.ZRDB.Results.SQLAlias, when
using traversal with a Z SQL Method.

1. Create a Z SQL Method named 'Sql' with one argument named 'my_arg':

      SELECT my_column AS my_column_alias
      FROM my_table
      WHERE my_primary_key = '<dtml-var my_arg sql_quote>'

   check 'Allow "simple" direct traversal' in Advanced.

2. Create a DTML Document named 'Leak':

      <dtml-var my_column_alias>

Then each time calling http://<host>/Sql/1/Leak, one additional Instance of
Shared.DC.ZRDB.Results.SQLAlias (a new str instance with value
'my_column_alias' being referenced by it's attribute _n)
will be created but never again freed unless Zope is restarted.

Notes:

- We used a python 2.4.4 build with Py_TRACE_REFS switched on.
- Cyclic garbage collection is active, but gc.collect() does not free the
  SQLAlias instances. (sys.getrefcount() says 4)

[Tres Seaver (tseaver)]

I can confirm that this leak is there, and has in fact caused
me to stop using the ZRDB.Results class altogether. Here is
where the problem arises:

  - For each result set (i.e., each invocation of a SQL method),
    the Results class creates a new class on-the-fly, named 'r'.
    It creates an uppercase and lowercase alias for each column
    in the result set's schema, and sets those aliases as
    attributes of the 'r' class, in order to get their '__of__'
    methods called when the alias is requested as an attribute
    of an instance of 'r'.

  - The cycle is thus betweeen the on-the-fly class, which
    is stashed as the '_class' attribute of the Results instance,
    and the alias(es). I don't know why that 'r' class is
    unreachable for GC, but it seems to be so. I also don't
    know how to write a test for the (non)existence of the
    cycle.

My workaround was to move to using a results class which did
not attempt to DWIM the uppercase and lowercase aliases at
all: the columns can only be accessed using the same spelling
that was used in the original SQL. BBB will make this harder
to do in the ZRDB.Results module.

[Artur Zaprzała (arturz)]

I also hit this leak after upgrading from Zope-2.9.4 to Zope-2.11.1. The attachment contains a simple but working fix (I hope someone will create a more elegant solution).

Here is a sample PageTemplate that will trigger a memory leak:
<div tal:define="row python:context.some_Z_SQL_Method()[0]" tal:content="row/some_column"></div>

Someone willing to fix this bug will find this stack trace of evaluation of tal:content expression useful:
...
  Module zope.tales.expressions, line 217, in __call__
  Module Products.PageTemplates.Expressions, line 124, in _eval
  Module zope.tales.expressions, line 124, in _eval
  Module Products.PageTemplates.Expressions, line 79, in boboAwareZopeTraverse
  Module zope.interface.declarations, line 375, in implementedByFallback
...

Another example that leaks memory:
from zope.traversing.interfaces import ITraversable
ITraversable(row, None) # row is an instance of Shared.DC.ZRDB.Results.r

ITraversable instance is created somwhere on the way of evaluating tal:content="row/some_column" expression. As a consequence the class Shared.DC.ZRDB.Results.r gets registered somewhere in ZOPE inteface machinery which prevents garbage collector from freeing it and other referenced objects (like Shared.DC.ZRDB.Results.SQLAlias objects).

Tracking references using gc.get_referrers() I found that zope.interface.adapter.AdapterLookup instance keeps cache that looks like this:
{
<InterfaceClass zope.traversing.interfaces.ITraversable>: {
    <implementedBy Shared.DC.ZRDB.Results.r>: <class 'zope.traversing.adapters.DefaultTraversable'>,
    <implementedBy Shared.DC.ZRDB.Results.r>: <class 'zope.traversing.adapters.DefaultTraversable'>,
    <implementedBy Shared.DC.ZRDB.Results.r>: <class 'zope.traversing.adapters.DefaultTraversable'>,
    ...
    },
...
}

On my production machine this bug leaks almost 2GB of memory daily. The fix for the class Shared.DC.ZRDB.Results.r is the most urgent, but other on-the-fly created classes are also affected.

[Tres Seaver (tseaver)]

A better fix might be to remove the pre-computed aliases (the source of the cycle), and define the class with a __getitem__ which case-normalizes requested keys.

[Tres Seaver (tseaver)]

The attached module has a Results class which provides a much stripped-down
form of the one in Shared.DC.ZRDB.Results. In particular:

 - It doesn't alias column names (the source of the memory leak).

 - It provides much more efficient mapping of a row index into a
   dictionary keyed by column name.

It loses a bunch of seldom used features of the original class (methods not
implemented).

To deal with the weird calling convention Shared.DC.ZRDB.DA uses to create
results, it contains a factory function, 'makeZRDBResults', which expects
to be passed a single tuple, (items, data).

You should be able to monkey-patch this factory in via something like::

  import Shared.DC.ZRDB.Connection
  Shared.DC.ZRDB.Connection.Results = makeZRDBResults
  import Shared.DC.ZRDB.DA
  Shared.DC.ZRDB.DA.Results = makeZRDBResults

[Tres Seaver (tseaver)]

Now tracked in https://github.com/zopefoundation/Products.ZSQLMethods/issues/10.