Django

Code

Ticket #11134 (closed: fixed)

Opened 10 months ago

Last modified 10 months ago

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

Reported by: Honza_Kral Assigned to: Honza_Kral
Milestone: 1.1 Component: Uncategorized
Version: SVN Keywords:
Cc: Triage Stage: Unreviewed
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

11134-against-10791.diff (2.3 kB) - added by Honza_Kral on 05/17/09 15:08:54.

Change History

05/17/09 15:08:54 changed by Honza_Kral

  • attachment 11134-against-10791.diff added.

05/18/09 06:31:42 changed by Honza_Kral

  • owner changed from nobody to Honza_Kral.
  • needs_better_patch changed.
  • needs_docs changed.
  • needs_tests changed.
  • milestone set to 1.1.

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

(follow-up: ↓ 3 ) 05/18/09 23:24:32 changed by gwilson

  • description changed.

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.

(in reply to: ↑ 2 ) 05/19/09 03:59:47 changed 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.

05/20/09 11:13:56 changed by jacob

  • status changed from new to closed.
  • resolution set to fixed.

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

05/20/09 11:19:26 changed 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.


Add/Change #11134 (Signals don't work properly when they are being disconnected during their processing)




Change Properties
Action