Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event callbacks which register/unregister other callbacks lead to surprising behavior #9447

Closed
craigcitro opened this issue May 8, 2016 · 4 comments · Fixed by #9453
Closed
Milestone

Comments

@craigcitro
Copy link
Contributor

tl;dr: the loop in this line isn't robust to register/unregister. What's the desired behavior?

The problem

We ran into a funny situation in colaboratory, involving post_run_cell callbacks which need to potentially add or remove other post_run_cell callbacks. Here's a really simplified version:

ip = get_ipython()
ev = ip.events

def func1(*unused_args):
    print 'invoking func1'
    ev.unregister('post_run_cell', func1)
    ev.register('post_run_cell', func3)

def func2(*unused_args):
    print 'invoking func2'
    ev.unregister('post_run_cell', func2)

def func3(*unused_args):
    print 'invoking func3'

ev.register('post_run_cell', func1)
ev.register('post_run_cell', func2)

In principle, this should invoke the three functions in order. In reality, it invokes func1 and func3, but not func2. Even worse, at the end of this cell's execution, the list of registered callbacks is [func2, func3], which is not the list of callbacks we saw execute. So func2, the only function that stays in the list of callbacks the whole time, is the only one we don't execute. 😉

The cause is easy to see after checking out this line:

  • on the first iteration of the loop, func1 is our callback, and [func1, func2] is our list of callbacks. func1 executes, and our list of callbacks is now [func2, func3].
  • the next loop iteration picks up at the second element of the list, namely func3. unfortunately, we've now skipped func2. sadface.

The lesson is, of course, never mutate a list as you iterate over it.

Potential solutions

I'm happy to send a PR, but first, I want to make sure we're on the same page about what the desired semantics are here.

I think we want to be flexible: given that the only exposed operations for callbacks are remove and append (and thankfully not reset), we can ensure

  1. we execute new callbacks appended to the list, and
  2. we skip removed callbacks if we haven't already executed them.

Other options all involve preventing this behavior. We could freeze state by instead making a copy of the list before we iterate over it, whence any modifications would only be visible on the next triggering of the event. Or we could add code to completely disallow modifications to the currently-executing list of callbacks for the duration of trigger, meaning a user would get an error if they tried.

Our use case

A common pattern we've used is "one-shot callbacks" -- a callback which, when invoked, removes itself from the list of callbacks. We use this for some forms of output manipulation, as well as occasionally to do one-time cleanup that needs to happen after the user's current code completes.

With the proposed solution, this is easy -- the body of a callback just includes a call to deregister itself. This currently works great, unless the one-shot callback is not the last callback in the list. (The example above was distilled from a variant of this -- a one-shot callback was registering another one-shot callback.) It also allows for forcing a callback to be idempotent -- register as many times as you'd like, and the first one can deregister any copies it finds.

With either of the other solutions, we're stuck basically inventing our own event system, and using it to handle cleanup.

@takluyver
Copy link
Member

Thanks for the thorough description!

In the abstract, I'd expect that newly registered handlers run the next time the event fires, but not for the event that's already being handled - i.e. we take a copy of the list of callbacks before running any of them.

That would still allow for a one-shot callback, I think - it would remove itself from the original list, and we'd continue iterating over the unchanged copy for the current event. It wouldn't allow one callback to remove another before it executes for the current event, but that seems like the kind of coupling that the event system shouldn't support: callbacks shouldn't depend on the order they're registered in.

@marksandler2
Copy link

Yes. That would probably the most natural behavior though if you notice the current implementation does exactly the opposite. It executes newly added callbacks but completely breaks down if one unregister during the callback.

If I were writing it from scratch I would probably go for that behavior but if you want to keep backwards compatibility one would need to do something more complicated.

@marksandler2
Copy link

FTR: We are happy to adopt Thomas's semantic, (of freezing the list)
just wanted to point out that it breaks existing behavior, however i am not aware of any significant use cases that relies on the behavior being broken..

@takluyver
Copy link
Member

Thanks Mark. I think I'm happy enough to say that one callback adding another to run on the same event is a corner case. I don't think the event system has seen much use yet, so I'd lean towards doing what makes most sense over striving to preserve compatibility.

craigcitro added a commit to craigcitro/ipython that referenced this issue May 11, 2016
Event callbacks can register or unregister new callbacks for the same event
while executing, and the previous triggering implementation allowed for event
callbacks to be inadvertently skipped.

The fix is to make a copy of the list of callbacks before executing any of
them. With this change, the resulting semantics are simple: any callbacks
registered before triggering are executed, and any new callbacks registered
are only visible at the next triggering of the event.

Note that this could potentially break existing callers who expected
newly-appended callbacks were immediately executed.

Fixes ipython#9447. Originally based on a patch by @marksandler2.

working on event callbacks
craigcitro added a commit to craigcitro/ipython that referenced this issue May 11, 2016
Event callbacks can register or unregister new callbacks for the same event
while executing, and the previous triggering implementation allowed for event
callbacks to be inadvertently skipped.

The fix is to make a copy of the list of callbacks before executing any of
them. With this change, the resulting semantics are simple: any callbacks
registered before triggering are executed, and any new callbacks registered
are only visible at the next triggering of the event.

Note that this could potentially break existing callers who expected
newly-appended callbacks were immediately executed.

Fixes ipython#9447. Originally based on a patch by @marksandler2.
@minrk minrk added this to the 4.3 milestone May 11, 2016
craigcitro added a commit to craigcitro/ipython that referenced this issue May 12, 2016
Event callbacks can register or unregister new callbacks for the same event
while executing, and the previous triggering implementation allowed for event
callbacks to be inadvertently skipped.

The fix is to make a copy of the list of callbacks before executing any of
them. With this change, the resulting semantics are simple: any callbacks
registered before triggering are executed, and any new callbacks registered
are only visible at the next triggering of the event.

Note that this could potentially break existing callers who expected
newly-appended callbacks were immediately executed.

Fixes ipython#9447. Originally based on a patch by @marksandler2.
@Carreau Carreau modified the milestones: 4.3, 5.0 Jun 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants