Opened 2 years ago

Closed 22 months ago

#5912 enhancement closed fixed (fixed)

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

Reported by: tom.prince Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Branch: branches/deferred-reference-clearing-5912
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

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.

Attachments (1)

update-existing-calls.diff (1.2 KB) - added by tom.prince 22 months ago.
This updates the existing examples to use this idiom.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 years ago by 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?

comment:2 Changed 22 months ago by thijs

  • Keywords documentation added

comment:3 Changed 22 months ago by glyph

  • Owner set to tom.prince

Tom, can you elaborate as per my previous comment?

comment:4 Changed 22 months ago by tom.prince

  • Owner changed from tom.prince to glyph

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.

comment:5 Changed 22 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/deferred-reference-clearing-5912

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

comment:6 Changed 22 months ago by exarkun

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

refs #5912

comment:7 Changed 22 months ago by exarkun

  • Keywords review added
  • Owner glyph deleted

Changed 22 months ago by tom.prince

This updates the existing examples to use this idiom.

comment:8 Changed 22 months ago by tom.prince

This seems sufficient to me.

comment:9 Changed 22 months ago by 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.

comment:10 follow-up: Changed 22 months ago by 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).

comment:11 in reply to: ↑ 10 Changed 22 months ago by 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

comment:12 Changed 22 months ago by cyli

  • Keywords review removed
  • Owner set to exarkun

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".
  1. 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.
  1. 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.
  1. Seems like it might be useful to link to the bit of documentation about non-garbage collected Deferreeds ?

comment:13 Changed 22 months ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

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.

comment:14 Changed 22 months ago by cyli

  • Keywords review removed
  • Owner set to exarkun

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!

comment:15 Changed 22 months ago by exarkun

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

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

Note: See TracTickets for help on using tickets.