Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#11134 closed (fixed)

Signals don't work properly when they are being disconnected during their processing

Reported by: Honza_Kral Owned by: Honza_Kral
Component: Uncategorized Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by gwilson)

This happens because disconnect() method does del self.receivers[index] while self.receivers are being iterated over in _live_receivers().

The attached patch has tests and a fix, the tests fail for me without the fix, passes after the fix is applied.

Attachments (1)

11134-against-10791.diff (2.3 KB) - added by Honza_Kral 5 years ago.

Download all attachments as: .zip

Change History (7)

Changed 5 years ago by Honza_Kral

comment:1 Changed 5 years ago by Honza_Kral

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to Honza_Kral
  • Patch needs improvement unset

Bugs shouold be fixed before release and this is simple and very isolated.

comment:2 follow-up: Changed 5 years ago by gwilson

  • Description modified (diff)

Honza, I've got a few questions about the use case you mentioned in IRC:

"The use case is that we have a FormField, that stores text in markup in external Model, and passes rendered text in its clean, then registers the post_save hook to save the markup somewhere to enable later editing."

You mention that, as shown in the attached tests, that the second receiver doesn't get run or removed and that this causes problems for the next instance that gets saved.

Well, don't you still have a problem if some other thread saves a different instance of the Model before save gets called on the target instance? In this case, the receivers would get called on the different instance, get removed from the post_save hook, then not get called when the target instance is saved. Or do you not care to be thread safe?

Also, what happens if this custom field cleans fine, but the form still had errors in other fields. In this case the receivers are still connected and could cause havoc on the next instance saved.

If all you are doing is registering receivers and then removing them, could you not simply call the receiver functions after you save the instance and not hook them up to the signal at all? For example, in a post_save method of the field that the form calls after successful validation.

comment:3 in reply to: ↑ 2 Changed 5 years ago by Honza_Kral

Replying to gwilson:

Honza, I've got a few questions about the use case you mentioned in IRC:

http://github.com/ella/django-markup

...
Well, don't you still have a problem if some other thread saves a different instance of the Model before save gets called on the target instance? In this case, the receivers would get called on the different instance, get removed from the post_save hook, then not get called when the target instance is saved. Or do you not care to be thread safe?

At the moment I don't care about thread safety and this is only my use case (which I agree is not well thought out and still in need of some refinement).

Also, what happens if this custom field cleans fine, but the form still had errors in other fields. In this case the receivers are still connected and could cause havoc on the next instance saved.

yes, it would fail horribly, but that's a bug in my app design

If all you are doing is registering receivers and then removing them, could you not simply call the receiver functions after you save the instance and not hook them up to the signal at all? For example, in a post_save method of the field that the form calls after successful validation.

I wanted to avoid this so that the form doesn't have to be aware of the field and special case it. I wanted to have a generic solution without any other part of the framework having to know anything about it. It is still a work in progress.

But that is just my little twisted use case, consider for example a signal handler for gathering statistics that will disconnect after given amount of time has passed, it would also cause some other signals not being called. The point is disconnecting signals from within signals SHOULD work, and if not, it should raise an exception or do something sensible. Making it work correctly seems to be the easier solution.

comment:4 Changed 5 years ago by jacob

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

(In [10831]) Fixed #11134: signals recievers that disconnect during their processing no longer mess things up for other handlers. Thanks, Honza Kral.

While I was at it I also cleaned up the formatting of the docstrings a bit.

comment:5 Changed 5 years ago by jacob

(In [10832]) [1.0.X] Fixed #11134: signals recievers that disconnect during their processing no longer mess things up for other handlers. Thanks, Honza Kral. Backport of [10831] from trunk.

comment:6 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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.