twisted / twisted

Event-driven networking engine written in Python.
https://www.twisted.org
Other
5.43k stars 1.14k forks source link

don't alter a list you're looping on! (woven) #8821

Closed twisted-trac closed 20 years ago

twisted-trac commented 20 years ago
aleaxit's avatar aleaxit reported
Trac ID trac#420
Type defect
Created 2003-12-04 21:29:43Z

Attachments:

Searchable metadata ``` trac-id__420 420 type__defect defect reporter__aleaxit aleaxit priority__high high milestone__ branch__ branch_author__ status__closed closed resolution__fixed fixed component__ keywords__ time__1070573383000000 1070573383000000 changetime__1071614299000000 1071614299000000 version__ owner__dp dp cc__itamarst cc__dp cc__aleaxit ```
twisted-trac commented 20 years ago
itamarst's avatar @itamarst commented
#!html
<pre>
This issue can be for the woven bugs, open another issue if you find other such
remove-in-loop bugs. THat should work.
</pre>
twisted-trac commented 20 years ago
dp's avatar dp commented
#!html
<pre>
Thank you very much for the bug report. Being the author of both Woven and Nevow, I second 
Itamar's suggestion that if you are starting from scratch that you attempt to use Nevow instead. 
Nevow is a clean reimplementation of the woven core ideas without the burden of the 
overarchitected and very buggy (as you noticed) woven implementation. Unfortunately, there is no 
documentation quite yet. I would be happy to help you get off the ground, and then answer any 
questions you have in real-time on irc (fzZzy on openprojects) or via email (dp at 
twistedmatrix.com)

There is a little bit of work to be done to make nevow "really" ready for use outside of quotient, 
without three-line workarounds. I will have some time around christmas to really concentrate on 
doing this extra-curricular nevow activity, and will also try to take some time to document.

In the meantime, this bug in woven should be fixed. A patch would be great, because otherwise I'm 
not sure when I'll find time to fix it.
</pre>
twisted-trac commented 20 years ago
aleaxit's avatar aleaxit commented
#!html
<pre>
There are some examples of this subtle but horrid bug in twisted code, for
example one at twisted.web.woven.model method notify.  If an item of the list
self.views (on which the code is looping) does get removed, the NEXT item will
wantonly be skipped (neither notified nor examined for removal).  Simplest
patch: loop on a COPY of the list if you need to be able to alterthe list!
</pre>
twisted-trac commented 20 years ago
itamarst's avatar @itamarst commented
#!html
<pre>
Please list all examples where you've seen this? Also if you're using Woven you
may want to look at nevow, which is nicer and will replace it someday
(http://stewstuff.com/doc/nevow.xhtml).
</pre>
twisted-trac commented 20 years ago
aleaxit's avatar aleaxit commented
#!html
<pre>
--- "Itamar Shtull-Trauring [Twisted issue tracker]"
&lt;twisted.roundup@...> wrote:
> 
> Itamar Shtull-Trauring &lt;itamar@...> added the comment:

Hi Itamar,

> Please list all examples where you've seen this? Also if you're

I don't have such a list at hand now -- typically, when I'm studying
twisted sources, what I'm trying to find out is exactly how something
is working, in order to fix problems in my own twisted-using code,
so it's hard to remember to write every such "side issue" down.  Is
it better for me to accumulate notes and post them in a bunch, or
just post one at a time as I encounter them?

> using Woven you
> may want to look at nevow, which is nicer and will replace it
> someday
> (http://stewstuff.com/doc/nevow.xhtml).

Thanks for the tip!  I had already noticed nevow mentioned and
praised, but hadn't found out this one doc page yet, and from just
the Quotient sources I hadn't been able to make heads or tails out
of it yet.  Rikard Anglerud (which I'm CC'ing) and I are indeed
starting a near-total rewrite of the web interface to Strakt's
CAPS framework apps, using Twisted (as other parts of CAPS have
been doing for quite a while) instead of Webware (which is how he
had coded the first generation of that web interface); and woven
is/was what we plan(ned) to use, but we'll definitely consider
nevow as an alternative -- so, thanks again for your suggestion!

Alex
</pre>
twisted-trac commented 20 years ago
radix's avatar @radix commented
#!html
<pre>
Please list specific bugs or close this issue.
</pre>
twisted-trac commented 20 years ago
itamarst's avatar @itamarst commented
#!html
<pre>
Probably you'd want different issues for different packages. Whatever makes
sense to you really.
</pre>
twisted-trac commented 20 years ago
aleaxit's avatar aleaxit commented
#!html
<pre>
I don't understand msg1658.  Isn't the "specific bug" I originally pointed out in 
my original post -- msg1653 -- i.e., the first for loop in method notify of 
twisted.web.woven.model -- enough to need to be fixed before this issue is 
considered closed?  Is there a specific format for identifying the errant code 
("line 120 in file ... in the Twisted-1.1.1rc1 sources") that I should follow?  Pls 
clarify, thanks.  As per Itamar's latest suggestion, I plan to open a different 
issue each time I notice this (or any other) coding bug -- I just thought it might 
be helpful to point out that I have noticed, though not written down the location 
of, other occurrences of the same general issue.  BTW, the same method has 
another serious bug a few code lines later (line 133), as it does a .remove(view) 
where view is quite unrelated to the loops and test being performed (it's just 
whatever happened to be the last idem of self.views in the previous loop). So, 
anyway, whatever form is preferred for communicating such coding issues, just 
let me know, and I'll try to follow it in the future - thanks! 

Alex
</pre>
twisted-trac commented 20 years ago
aleaxit's avatar aleaxit commented
#!html
<pre>
I'm attaching the patch as requested -- I've downloaded the Twisted CVS, carefully 
reviewed all of the woven package, and found the error only in model.py (in several 
spots, though).  I've fixed things in the safest way, by looping on a copy list(L) of each 
list L that may be modified in the loop body.  Given the specific purpose of this code 
(which is just to remove weakrefs whose referent has disappeared) I suspect a 
sounder fix would be to use e.g. a weakref.WeakValueDictionary, instead of a list 
of weak references -- however, there are subtle issues there (no ordering, referents 
must not disappear during a loop, etc) so I played it safe instead. 

Tx for the offer to help with newov -- I did look at the docs Itamar pointed out in 
msg1656, and I'll take you up at once on your kind offer by separate email to 
dp at twistedmatrix.com as you indicate (so I can CC Rikard right back in:-). 

Alex
</pre>
twisted-trac commented 20 years ago
dp's avatar dp commented
#!html
<pre>
Patch applied, thank you.
</pre>