As I said, fair enough. :-)
On Tue, Aug 12, 2014 at 6:57 PM, Michael Bayer <mike...@zzzcomputing.com> wrote: > > 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. > 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) > > > > I thought of that as well. But to make this bulletproof would be > difficult. An event dispatcher isn’t necessarily single-thread use, for > starters. It is surprising that the event.remove() operation would appear > to fail within the handler itself, and whether or not the remove succeeded > could not be reported until later. The remove() operation is not a > simple one to begin with (it is quite involved in fact), and supporting > mutation of a collection while iteration proceeds is just an edgy case and > it’s a lot simpler if we just disallow it in a simple way. > > > > > 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 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.
