On Aug 12, 2014, at 12:28 PM, Jonathan Stoppani <jonathan.stopp...@gmail.com> wrote:
> Hello Michael, > > Thank you for the detailed analysis. Your conclusions look fair to me, and if > the deque approach is implemented we "may" even omit the removal issue from > the documentation. It's easy to add a short note to the docs for remove() that inline removal is not supported. We will do that; but it's the kind of doc note people will miss. That's why the exception raise is great, it disallows silent failure and makes the restriction clear (well, at least it would direct people back to the remove() documentation to see our note). It's kind of a modified case of "ask forgiveness later rather than permission first". > I'll use the suggested approach with a single event for my needs. > > Another approach could be to just set a flag when an event listener is > removed and actually remove it only once the all events have fired, along the > lines of (pseudocode): > > > class _CompoundListener(_HasParentDispatchDescriptor): > def __call__(self, *args, **kw): > """Execute this event.""" > > for fn in self.listeners: > fn(*args, **kw) > > for e in self._removed: > event._remove(e) > > > On Tue, Aug 12, 2014 at 6:17 PM, Michael Bayer <mike...@zzzcomputing.com> > wrote: > > it's a pretty ordinary issue, a patch such as this allows it to succeed: > > diff --git a/lib/sqlalchemy/event/attr.py b/lib/sqlalchemy/event/attr.py > index 7641b59..6bf1b34 100644 > --- a/lib/sqlalchemy/event/attr.py > +++ b/lib/sqlalchemy/event/attr.py > @@ -256,7 +256,7 @@ class _CompoundListener(_HasParentDispatchDescriptor): > > for fn in self.parent_listeners: > fn(*args, **kw) > - for fn in self.listeners: > + for fn in list(self.listeners): > fn(*args, **kw) > > def __len__(self): > > > that is, if you are iterating a Python list and remove an item from it, the > iterator gets out of whack. It's unfortunately not as kind to us as the > dictionary, which at least raises an exception when this occurs. > > However. This means we need to add list() calls in several places, adding > on average two function calls for every event invocation throughout the > system in order to suit a use case that is extremely rare and IMO not really > necessary. > > In fact, to fix this "correctly" adds even more overhead... > > --- a/lib/sqlalchemy/event/attr.py > +++ b/lib/sqlalchemy/event/attr.py > @@ -254,16 +254,14 @@ class _CompoundListener(_HasParentDispatchDescriptor): > def __call__(self, *args, **kw): > """Execute this event.""" > > - for fn in self.parent_listeners: > - fn(*args, **kw) > - for fn in self.listeners: > + for fn in self: > fn(*args, **kw) > > def __len__(self): > return len(self.parent_listeners) + len(self.listeners) > > def __iter__(self): > - return chain(self.parent_listeners, self.listeners) > + return iter(list(chain(self.parent_listeners, self.listeners))) > > def __bool__(self): > return bool(self.listeners or self.parent_listeners) > > > that means, __call__ adds about four function calls now, plus the creation of > a new list. there's be some more of those elsewhere in that module as well. > > So in that way this is the worst kind of issue, the choice between silent > failure in an extremely unlikely case vs. unnecessary performance overhead > for millions of applications. We get more than these than I'd prefer. > Plus if we are supporting this, then we have to add tests all over the place > for removing events inside of them under a myriad of scenarios. > > Re: not necessary, just use a single listener, and maintain a sub-structure > that indicates when you need to take action or not, e.g. a counter, a > collection of callables, whatever. Removal of listeners wasn't intended to > be that much of an on-the-fly use case. > > As far as what action to take, I'd love if there were some easy way to get > these lists to just raise an exception right now if they change on iteration, > but that would mean a custom list class and then function call overhead. > That would at least make the restriction self-documenting. > > Seems like a deque() (native, super fast) does this! Hm! I like this a > lot! the test spits out: > > RuntimeError: deque mutated during iteration > > I've created > https://bitbucket.org/zzzeek/sqlalchemy/issue/3163/use-deque-for-event-lists-so-that-remove > with that patch, for 1.0 as it is a behavior change. > > For now, try to not rely upon removing the event within it like that. > removal is not a cheap operation in any case. > > > > > > > On Aug 12, 2014, at 8:22 AM, GaretJax <jonathan.stopp...@gmail.com> wrote: > >> Hello, >> >> I have to run an event handler after a session was flushed and I run into >> some issues when removing these handlers once they were executed from inside >> the handlers themselves. >> >> At the following location there is a gist illustrating my issue: >> https://gist.github.com/GaretJax/b78bd97434992a8ebf26 >> I know it is a rather unusual scenario, but I'd like to know if this is a >> bug or if I missed a point in the documentation about listeners removal from >> inside the listener itself. >> >> P.S.: The same code without explicit event.remove call but with once=True >> during registration fails with an assertion error on event/registry.py", >> line 74, in _stored_in_collection: assert dispatch_reg[owner_ref] == >> listen_ref >> >> Best, >> Jonathan >> >> -- >> You received this message because you are subscribed to the Google Groups >> "sqlalchemy" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email tosqlalchemy+unsubscr...@googlegroups.com. >> >> To post to this group, send email to sqlalchemy@googlegroups.com. >> Visit this group at http://groups.google.com/group/sqlalchemy. >> For more options, visit https://groups.google.com/d/optout. > > > -- > You received this message because you are subscribed to a topic in the Google > Groups "sqlalchemy" group. > To unsubscribe from this topic, visit > https://groups.google.com/d/topic/sqlalchemy/k45PEofiZdk/unsubscribe. > To unsubscribe from this group and all its topics, send an email to > sqlalchemy+unsubscr...@googlegroups.com. > To post to this group, send email to sqlalchemy@googlegroups.com. > Visit this group at http://groups.google.com/group/sqlalchemy. > For more options, visit https://groups.google.com/d/optout. > > > > -- > ~jonathan > > -- > You received this message because you are subscribed to the Google Groups > "sqlalchemy" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to sqlalchemy+unsubscr...@googlegroups.com. > To post to this group, send email to sqlalchemy@googlegroups.com. > Visit this group at http://groups.google.com/group/sqlalchemy. > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "sqlalchemy" group. To unsubscribe from this group and stop receiving emails from it, send an email to sqlalchemy+unsubscr...@googlegroups.com. To post to this group, send email to sqlalchemy@googlegroups.com. Visit this group at http://groups.google.com/group/sqlalchemy. For more options, visit https://groups.google.com/d/optout.
