AttributeError in sqlalchemy.ext.declarative.clsregistry._MultipleClassMarker.add_item

Issue #3208 resolved
Scott Milliken created an issue

We've been getting occasional AttributeError exceptions raised from the sqlalchemy.ext.declarative.clsregistry._MultipleClassMarker.add_item method as it tries to access a __module__ attribute here: https://github.com/zzzeek/sqlalchemy/blob/28dd15081db4b7e978fa7a187c3aef1c0e4ad4e3/lib/sqlalchemy/ext/declarative/clsregistry.py#L106.

I haven't been able to identify the root cause or reliably reproduce the bug, but we are doing something fancy that's almost certainly related.

Using SQLAlchemy 0.9.6, Python 2.7.2, running inside Apache2.

Stacktrace:

 Module ourmodule1:37 in __init__
<<
>>  partition = aliased(get_partition_model(session, PartitionedTable))
Module ourmodule2:203 in get_partition_model
Module ourmodule2:190 in cons_model
Module sqlalchemy.ext.declarative.api:53 in __init__
<<      def __init__(cls, classname, bases, dict_):
               if '_decl_class_registry' not in cls.__dict__:
                   _as_declarative(cls, classname, cls.__dict__)
               type.__init__(cls, classname, bases, dict_)
>>  _as_declarative(cls, classname, cls.__dict__)
Module sqlalchemy.ext.declarative.base:148 in _as_declarative
<<          table_args = None

           clsregistry.add_class(classname, cls)
           our_stuff = util.OrderedDict()
>>  clsregistry.add_class(classname, cls)
Module sqlalchemy.ext.declarative.clsregistry:64 in add_class
<<          for token in tokens:
                   module = module.get_module(token)
               module.add_class(classname, cls)
>>  module.add_class(classname, cls)
Module sqlalchemy.ext.declarative.clsregistry:160 in add_class
<<          if name in self.contents:
                   existing = self.contents[name]
                   existing.add_item(cls)
               else:
                   existing = self.contents[name] = \
>>  existing.add_item(cls)
Module sqlalchemy.ext.declarative.clsregistry:105 in add_item
<<      def add_item(self, item):
               modules = set([cls().__module__ for cls in self.contents])
               if item.__module__ in modules:
                   util.warn(
>>  modules = set([cls().__module__ for cls in self.contents])
AttributeError: 'NoneType' object has no attribute '__module__'

The fanciness I mentioned is the get_partition_model function referenced in the stacktrace. The purpose of this function is to take the SQLAlchemy model for a PostgreSQL paritioned table (using table inheritance), and return a new SQLAlchemy model for the named partition. We cannot use SQLAlchemy's inheritance facilities because there's no discriminator column in the table partitions we can use. Here's the implementation of this function:

def get_partition_model(session, model, partition_id=None):
    '''
    Get a model specialized for the child partition identified by @partition_id.

    Assumes partitions are named like '%(parent_table)s_%(partition_id)s'.

    If @partition_id is None, then the partition with the latest lexical sort will be returned.
    '''
    def cons_model(partition_name):
        classname = str('%s%s' % (model.__name__, partition_name.split('_')[-1]))
        if classname in model._decl_class_registry:
            return model._decl_class_registry[classname]
        newtable = copy.copy(model.__table__)
        newtable.name = partition_name
        for col in newtable.columns:
            col.table = newtable
        return type(classname, (model,), {
            '__table__': newtable,
            '__tablename__': partition_name,
            '__mapper_args__': {'concrete': True}
        })
    tablename = model.__table__.name
    # NB: get_child_tables returns a list of partition tables using pg_class and pg_inherits
    partitions = get_child_tables(session, tablename)
    if not partitions:
        return
    if partition_id:
        partition_name = '%s_%s' % (tablename, partition_id)
        if partition_name not in partitions:
            raise ValueError('No such partition %r for %r' % (partition_id, tablename))
    else:
        partition_name = max(partitions)
    return cons_model(partition_name)

This code works approximately 100% of the time, but on rare occasions triggers the AttributeError mentioned. If I were to guess, I'd assume there's a race condition/thread safety issue (not neccessarily in SQLAlchemy).

Admittedly, this is a poor bug report since we're doing something fancy with dynamically created models, and I haven't given you any way to reproduce this. I thought I'd report it anyway in case it's obvious to you what's going on or if it invalidates some assumption in the code.

Comments (5)

  1. Mike Bayer repo owner

    OK well that cls() there is a weakref, so if it's returning None that means the weakref was garbage collected. What happens next is that _remove_item gets called, but if that hasn't been called yet before something calls add_item(), that would cause this.

    Python's GC can be doing this asynchronously so this kind of thing can happen, however, it indicates you have a declarative class somewhere that is being garbage collected, which is the odd case here.

    But it doesn't matter, because this is all just to emit a warning and isn't so critical, and the loop there can just skip those references that aren't active.

    the upcoming changeset will close this issue as fixed, if your issue persists with that changeset then reopen.

  2. Mike Bayer repo owner
    • Fixed an unlikely race condition observed in some exotic end-user setups, where the attempt to check for "duplicate class name" in declarative would hit upon a not-totally-cleaned-up weak reference related to some other class being removed; the check here now ensures the weakref still references an object before calling upon it further. fixes #3208

    → <<cset c7ec21b29e92>>

  3. Mike Bayer repo owner
    • Fixed an unlikely race condition observed in some exotic end-user setups, where the attempt to check for "duplicate class name" in declarative would hit upon a not-totally-cleaned-up weak reference related to some other class being removed; the check here now ensures the weakref still references an object before calling upon it further. fixes #3208

    → <<cset 80cd802296af>>

  4. Scott Milliken reporter

    Your hypothesis seems like the most reasonable cause. Running your fix in production for lack of a way to reproduce, but I'm certain that'll address it.

    Appreciate you taking the time to look into this!

  5. Log in to comment