removal flaw in django.dispatch.Signal.disconnect
|Reported by:||(removed)||Owned by:||nobody|
|Has patch:||no||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
Eyeballing cleanup in signals, spotted a bit of whackyness trying to cleanse a list via enumerate usage; presume the author tried removing from the list directory (which blows up since that invalidates the listiter) and came up w/ this instead. Flaw with it is that enumerate continues to increase the index even if you change the list under it's feet- in other words, you skip the item immediately following a deletion. One reason list comps are usually better in this case (although dependant on # of items and deletions of course).
So... that should be using list.remove and catching ValueError where it doesn't exist. Quite a bit faster due to shifting comparison to cpy, and stopping the search after the first find.
Clarifying the latter, a quick read of Signal.connect shows it disallows redundant connections to the same 'key'; meaning that the enumerate code in place right now is daft for continuing it's search for more to wipe- essentially a mismatch between connect/disconnect (guessing it crept in when someone tweaking signal.connect to disallow redundant registrations).
for idx, (r_key, _) in self.receivers: if r_key == lookup_key: del self.receives[idx]
try: self.receivers.remove(lookup_key) except ValueError: pass
Change History (6)
comment:1 Changed 6 years ago by jacob
- milestone set to 1.1
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
- Triage Stage changed from Unreviewed to Accepted