Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#9989 closed (fixed)

removal flaw in django.dispatch.Signal.disconnect

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

Description

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).

basically convert

for idx, (r_key, _) in self.receivers:
 if r_key == lookup_key:
  del self.receives[idx]

into

try:
 self.receivers.remove(lookup_key)
except ValueError:
 pass

'nuff said.

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

comment:2 Changed 6 years ago by russellm

  • Component changed from Uncategorized to Core framework

comment:3 Changed 6 years ago by jacob

I'm not sure it's quite that straightforward: when I make this change, the signal disconnecting at the end of the signals doctest fail to get disconnected. To me that indicates more than just a semantic difference between the two ways of doing this.

comment:4 Changed 6 years ago by jacob

Ah, that's why -- recievers is a list of ((to, from), receiver), not a dict. And since receiver is a weakref we can't exactly reconstruct it. So I'm failure sure the way we're doing it is the only way we can; just need to move the enumerate outside of the loop, perhaps.

comment:5 Changed 6 years ago by jacob

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

(In [10398]) Fixed #9989: fixed a subtle edge case where removing signals could break. Thanks, ferringb.

comment:6 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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