#21085 closed Bug (invalid)

Race condition in BoundMethodWeakref used by signal mechanism.

Reported by: grahamd Owned by: nobody
Component: Core (Other) Version: 1.5
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by akaariai)

In the __new__ method of BoundMethodWeakref at:

https://github.com/django/django/blob/master/django/dispatch/saferef.py#L73

there is a potential threading race condition.

The code is:

    def __new__( cls, target, onDelete=None, *arguments,**named ):
        """Create new instance or return current instance

        Basically this method of construction allows us to
        short-circuit creation of references to already-
        referenced instance methods.  The key corresponding
        to the target is calculated, and if there is already
        an existing reference, that is returned, with its
        deletionMethods attribute updated.  Otherwise the
        new instance is created and registered in the table
        of already-referenced methods.
        """
        key = cls.calculateKey(target)
        current =cls._allInstances.get(key)
        if current is not None:
            current.deletionMethods.append( onDelete)
            return current
        else:
            base = super( BoundMethodWeakref, cls).__new__( cls )
            cls._allInstances[key] = base
            base.__init__( target, onDelete, *arguments,**named)
            return base

The problem is that multiple threads could at the same time invoke cls._allInstances.get(key) and get None.

They will both then create instances of BoundMethodWeakref but only one will win as far as updating cls._allInstances.

Because all the onDelete callbacks are stored in the BoundMethodWeakref instance which is added to the cache. the thread who lost as far as getting their instance in the cache, will have its onDelete lost.

Code should in the else clause instead do something like:

            base = super( BoundMethodWeakref, cls).__new__( cls )
            base.__init__( target, onDelete, *arguments,**named)

            instance = cls._allInstances.setdefault(key, base)

            if instance is not base:
                instance.deletionMethods.append(onDelete)

            return instance

Change History (4)

comment:1 Changed 21 months ago by akaariai

  • Component changed from Uncategorized to Core (Other)
  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:2 Changed 21 months ago by akaariai

See also #19511.

comment:3 Changed 19 months ago by vajrasky

Is this bug related with this issue? With python 3.4:

$ python runtests.py null_fk
Creating test database for alias 'default'...
Creating test database for alias 'other'...
..
----------------------------------------------------------------------
Ran 2 tests in 0.063s

OK
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...
Exception ignored in: <bound method Signal._remove_receiver of <django.dispatch.dispatcher.Signal object at 0x7f662b75b330>>
Traceback (most recent call last):
  File "/home/sky/Code/python/django/django/dispatch/dispatcher.py", line 276, in _remove_receiver
TypeError: 'NoneType' object is not callable
Exception ignored in: <bound method Signal._remove_receiver of <django.dispatch.dispatcher.Signal object at 0x7f662b1be6d8>>
Traceback (most recent call last):
  File "/home/sky/Code/python/django/django/dispatch/dispatcher.py", line 276, in _remove_receiver
TypeError: 'NoneType' object is not callable
Exception ignored in: <bound method Signal._remove_receiver of <django.dispatch.dispatcher.Signal object at 0x7f662b1be6d8>>
Traceback (most recent call last):
...omitted...

comment:4 Changed 16 months ago by mjtamlyn

  • Resolution set to invalid
  • Status changed from new to closed

Our implementation of weakrefs has now gone in favour of (backported) code from python 3.4. This should hopefully no longer be an issue.

Note: See TracTickets for help on using tickets.
Back to Top