Code

Opened 4 years ago

Closed 3 months ago

#13734 closed Cleanup/optimization (fixed)

Simpler implementation of django.dispatch.dispatcher.Signal._remove_receiver

Reported by: suzaku Owned by: suzaku
Component: Core (Other) Version: 1.2
Severity: Normal Keywords: dispatcher, signal
Cc: russellm, satorulogic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In Django 1.2.1, the _remove_receiver method of the django.dispatch.dispatcher.Signal class is implemented as:

    def _remove_receiver(self, receiver):
        """
        Remove dead receivers from connections.
        """

        to_remove = []
        for key, connected_receiver in self.receivers:
            if connected_receiver == receiver:
                to_remove.append(key)
        for key in to_remove:
            for idx, (r_key, _) in enumerate(self.receivers):
                if r_key == key:
                    del self.receivers[idx]

I think this can be simplified by iterating the receivers list reversely and removing the specified receiver by index:

    def _remove_receiver(self, receiver):
        """
        Remove dead receivers from connections.
        """
        for idx in xrange(len(self.receivers)-1, -1, -1):
            (_, connected_receiver) = self.receivers[idx]
            if connected_receiver == receiver:
                del self.receivers[idx]

Attachments (1)

simplified_remove.diff (754 bytes) - added by suzaku 4 years ago.

Download all attachments as: .zip

Change History (10)

Changed 4 years ago by suzaku

comment:1 Changed 4 years ago by suzaku

  • Component changed from Uncategorized to Core framework
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 follow-up: Changed 4 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Marking as 'needs tests' because I'm not completely certain of the extent to which this disconnection behavior is tested at present; if it isn't, an optimizatin like this should is a reasonable excuse to beef up the tests.

If it turns out I'm mistaken and this *is* tested, feel free to leave a note to that effect and drop the flag.

comment:3 in reply to: ↑ 2 Changed 4 years ago by suzaku

  • Cc russellm added
  • Needs tests unset

Replying to russellm:

Marking as 'needs tests' because I'm not completely certain of the extent to which this disconnection behavior is tested at present; if it isn't, an optimizatin like this should is a reasonable excuse to beef up the tests.

If it turns out I'm mistaken and this *is* tested, feel free to leave a note to that effect and drop the flag.

This is not a disconnection behavior. Rather, _remove_receiver is registered as a weakref callback when the receiver object is garbage collected.
I think this behavior is tested here http://code.djangoproject.com/browser/django/trunk/tests/regressiontests/dispatch/tests/test_dispatcher.py#L55.

comment:4 Changed 4 years ago by suzaku

  • Owner changed from nobody to suzaku

comment:5 Changed 4 years ago by suzaku

  • Cc satorulogic@… added

comment:6 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:7 Changed 3 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

simplified_remove.diff fails to apply cleanly on to trunk

comment:8 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 3 months ago by claudep

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

That part of the code has changed a lot recently, in particular with 52cad43bc3d3126fcf7e08582373c12e88895cd3 and c29d6f767691cceb9964c0d212e01281ac6721d3
This report is now obsolete.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.