Explain `d, self.deferred = self.deferred, None` idiom

[tom.prince]

This common idiom should be explained.

if self.deferred is not None:
     d, self.deferred = self.deferred, None

That code sets d to self.deferred at the same time as setting self.deferred to None. That way, if the code is called again later, then self.deferred, the condition in the if evaluates to False. Also, it gets rid of the reference to the deferred, allow it and all its callbacks (including the captured closures) to be garbage collected.

[glyph]

My first thought is "why bother to explain this, it's the Python documentation's job, this is a Python idiom".

But then my second thought is, "the callbacks will be collected by virtue of the Deferred firing, I think you're referring to the closed-over stack state of the resulting Failure if it fails". So maybe the idiom does in fact warrant some explanation. But, where is this idiom used in documentation?

[glyph]

Tom, can you elaborate as per my previous comment?

[tom.prince]

It appears that the idiom doesn't currently appear in the twisted docs (although there are a couple of place where it might be warranted: doc/web/howto/client.xhtml and doc/core/howto/process.xhtml). The specific example that prompted this ticket was  http://krondo.com/?p=1778

Here is the IRC transcript. (This was after Freeder indicated that the official documentation wasn't sufficent.

<Freeder> if self.deferred is not None:
<Freeder>     d, self.deferred = self.deferred, None
<Freeder> I thought each factory instance runs in its own memory space... how does that prevent a deferral from running twice?
<_habnabit> ... huh?
<Freeder> or why do we even need to clear that out?
<_habnabit> where's that code?
<Freeder> its from an example, but says its a pattern observed several times in the source
<Freeder> thats after the deferred is fired
<_habnabit> could you give a link to it? I'd just like to see the full context
<Freeder> guess I'm just wondering if thats style or best practice
<Freeder> as I'm not sure why that would need to be done
<glyph> Freeder: can you link to the specific example?
<glyph> Freeder: it might be an illustration of some style or best practice but without seeing the whole example it's hard to know why it's doing  
that.
<Freeder> glyph: http://krondo.com/?p=1778
<Freeder> about a page down or so
<tomprince> Freeder: That code sets d to self.deferred at the same time as setting self.deferred to None. That way, if the code is called again later, then self.deferred, the condition in the if evaluates to False.
<Freeder I understand physically what the code is doing, just not sure why its necessary, ie the style vs best practice part of my question. I thought a deferred can only be called once (to which maybe Im' wrong), but if so, why bother releasing it like that? It's probably semantics, but would like to know if I misunderstand something
<_habnabit> Freeder, if you call .callback() on a deferred after it's been called back, it raises an error. in this code, it avoids the error because it only matters which one was called _first_
<_habnabit> Freeder, as an added bonus, it doesn't keep an extra reference to the deferred around. if a deferred is still going through errbacks instead of callbacks when it's garbage collected, it'll log an error
<Freeder> _habnabit: so it is only an error avoidance thing? Cool. Thanks.

[exarkun]

(In [35873]) Branching to 'deferred-reference-clearing-5912'

[exarkun]

(In [35874]) Clear an attribute referring to a Deferred. Explain why.

refs #5912

[exarkun]

 Build results

[tom.prince]

This updates the existing examples to use this idiom.

[tom.prince]

This seems sufficient to me.

[exarkun]

Notice I already created a branch and checked changes into it. Attaching a patch is out-of-workflow and means that it's no longer clear whether a reviewer can look at this ticket or what they should look at. Please don't do that.

To reviewers, please disregard the patch and look at the branch (unless the state of the ticket has changed following this comment). To Tom, please clarify what you hoped to happen next with this ticket. Thanks.

[tom.prince]

I think the description of the idiom is good. I also think that examples where the use of the idiom is appropriate should be changed to use the idiom (which is what the patch does).

exarkun: I don't see anywhere in http://twistedmatrix.com/trac/wiki/TwistedDevelopment http://twistedmatrix.com/trac/wiki/UltimateQualityDevelopmentSystem or http://twistedmatrix.com/trac/wiki/UltimateQualityDevelopmentSystem forbidding attach patches. I know, although I don't see mentioned anywhere, that attached patches should be against a branch, if one exists (which this is).

[glyph]

Replying to tom.prince:

exarkun: I don't see anywhere in http://twistedmatrix.com/trac/wiki/TwistedDevelopment http://twistedmatrix.com/trac/wiki/UltimateQualityDevelopmentSystem or http://twistedmatrix.com/trac/wiki/UltimateQualityDevelopmentSystem forbidding attach patches. I know, although I don't see mentioned anywhere, that attached patches should be against a branch, if one exists (which this is).

Tom,

The problem is not that you attached a patch; attaching patches is fine. In fact you can feel free to make patches against trunk or against an existing branch, whichever you like, as long as you say what it is.

The problem is that the patch was already in review, and you didn't review it; you attached a patch which may have intended to be applied on top of the branch, may have been intended as a separate submission for review (an alternate approach to the problem).

If a ticket is in review, you should review it, or you should wait for someone else to review it. Submitting more code without first discussing it with the original author can cause all sorts of problems. For example, when someone now reviews this ticket, they might review the patch, or the branch, or the combination of the two, and they don't know if they're responding to you or to exarkun or both.

So the modification was "out of workflow" because the "review" state goes to the "merged" state or to the "needs improvement" state, not to the "here are some extra changes" state. If you want to do something like this which is not described by the development process, you have to be super explicit about exactly what you want to be done with the patch, and who you would like to do it. So rather than "this looks sufficient to me" (did you intend that to be a review comment? you can't review a ticket that you've been an author on...) you would need to say "Reviewer, please apply this patch on top of exarkun's branch; I will volunteer to deal with any feedback on the branch". (But you should talk to exarkun first, even if just on a comment on this ticket, before stealing the ticket.)

Thanks,

-glyph

[cyli]

Awesome! This was a subtle point that it took me a while to realize, so I really appreciate the explanation.

1. In the explanation, you mention "... it avoids any chance Getter.gotResults will accidentally fire the same Deferred more than one (which would result in an error) ...". This should be changed to "more than once".

2. If Getter.gotResults got called in such a way that it could have fired the same Deferred more than once, then the way you've changed the code it'd still result in an error since self.d would be None and you'd be attempting to callback a None, rather than a Deferred. I'm basically saying in a roundabout way that the example code is not particularly robust, and it may be confusing for people to exchange one error for another.

3. It might also be clearer to refer to self.d in the first sentence of your explanation. I realize that you specify "the d attribute", but since d is emphasized and "attribute" is not, it took me a sec to realize you were talking about self.d or <Getter instance.d, not the local variable d.

4. Seems like it might be useful to link to the bit of documentation about non-garbage collected Deferreeds ?

[exarkun]

Thanks. I addressed the first three points in r36231. I didn't address the last point because I don't want to add links to the wiki to the howto documentation and because I feel like this section of the documentation is crowded enough already and I can't see a good way to edit in a new topic without totally destroying the flow.

[cyli]

I suppose not adding in the 4th point makes sense - it brings in other stuff like setDebugging and such. I just thought it would be useful to include why caring about making things easier for the GC is a good idea, but perhaps that should go elsewhere.

Looks good to me. Please merge!

[exarkun]

(In [36235]) Merge deferred-reference-clearing-5912

Author: exarkun Reviewer: cyli Fixes: #5912

Add a section to the Deferred howto explaining why dropping a reference to a Deferred before firing it is a good idea.