Wrong ExitStack Callback recipe

[Denaun]

BPO	32145
Nosy	@warsaw, @ncoghlan, @Denaun

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-11-27.08:34:13.573>
labels = ['type-bug', '3.7', 'docs']
title = 'Wrong ExitStack Callback recipe'
updated_at = <Date 2018-01-02.07:21:53.341>
user = 'https://github.com/Denaun'

bugs.python.org fields:

activity = <Date 2018-01-02.07:21:53.341>
actor = 'ncoghlan'
assignee = 'docs@python'
closed = False
closed_date = None
closer = None
components = ['Documentation']
creation = <Date 2017-11-27.08:34:13.573>
creator = 'Denaun'
dependencies = []
files = []
hgrepos = []
issue_num = 32145
keywords = []
message_count = 8.0
messages = ['307037', '307364', '308011', '309046', '309064', '309172', '309176', '309357']
nosy_count = 4.0
nosy_names = ['barry', 'ncoghlan', 'docs@python', 'Denaun']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32145'
versions = ['Python 3.6', 'Python 3.7']

[Denaun]

The documentation for contextlib.ExitStack (https://docs.python.org/3.7/library/contextlib.html#replacing-any-use-of-try-finally-and-flag-variables) shows an helper class (Callback) to perform cleanup using a callback.
Problem is, Callback.cancel() calls ExitStack.pop_all(), which in turn calls type(self)(), this is not a valid constructor for Callback, since it always expects at least one element.

(I have marked only Python 3.4 and 3.6 since those are the versions where I verified this, but it likely applies to every version.)

[ncoghlan]

Hmm, I think that may actually qualify as a bug in the pop_all() implementation: https://docs.python.org/3/library/contextlib.html#contextlib.ExitStack.pop_all states that it returns an ExitStack instance, not an instance of the current type.

For 3.6 (and hence the online docs), we can fix the recipe to allow for callback=None (with the expectation that the callback will be added afterwards).

Barry, I'd be interested in your thoughts on what to do for 3.7+ - we can either leave the current behaviour alone, and amend the documentation, or else change the code to call ExitStack directly, rather than type(self).

I'm leaning towards only changing the docs as being lower risk - folks may be relying on the current behaviour, so changing it may break their code, whereas changing the docs doesn't risk breaking anything.

[warsaw]

I wasn't even aware that pop_all() tries to create the concrete subtype. I wonder how common subclassing ExitStack is in practice. (Of course, the answer is that we'll never know.)

For 3.7, we should probably delegate instance creation to a new non-private method, e.g.

    new_stack = self._make_instance()

so *if* you subclass, you can create the new ExitStack subclass instances any way you want. I would then change the default implementation to make an ExitStack() explicitly, as per the current documentation.

+1 for fixing the recipe and not changing the code for 3.6. You might want to add a note describing the current behavior for <= 3.6 and indicating that this will change for 3.7+

Finally, while you're at it, is there any reason not to change the recipe to use:

super().__init__()

instead?

[ncoghlan]

Regarding super().__init__(): I added ExitStack to contextlib2 first, so I was thinking in the Py2/3 compatible subset when I wrote the original docs. We can freely change that for the standard library recipes.

Regarding the "how to create a copy" question, while I don't especially like the notion of adding a dedicated semi-public copy method, I think you're right that it may be the best option:

1. Full pickle/copy module support doesn't make sense, since exit stacks are inherently stateful - you can't save them to use later, since you'd need to repeat the setup steps as well, but we don't keep track of what the setup steps actually *were*.

2. stack.pop_all() was designed such that if you clone the stack, you ensure that the original stack won't call the cleanup callbacks any more. If we were to add a fully public stack.copy() method (or copy.copy(stack) support via stack.__copy__()), then we'd lose that deliberate nudge, and put folks more at risk of "double-free" style bugs, where they run the cleanup functions multiple times.

3. A for-subclasses-only "self._clone()" API could work as follows:

    def _clone(self, new_instance=None):
        if new_instance is None:
            new_instance = type(self)()
        # Clone state here
        return new_instance

Then subclasses could override *just* the instance creation part by doing:

    def _clone(self):
        return super()._clone(self._make_instance())

While also being free to add their own additional state copying code if needed.

[warsaw]

On Dec 25, 2017, at 18:51, Nick Coghlan <report@bugs.python.org> wrote:

3. A for-subclasses-only "self._clone()" API could work as follows:

   def _clone(self, new_instance=None):
   if new_instance is None:
   new_instance = type(self)()
   # Clone state here
   return new_instance

Then subclasses could override *just* the instance creation part by doing:

def _clone(self):
return super()._clone(self._make_instance())

While also being free to add their own additional state copying code if needed.

So _make_instance() wouldn’t be part of ExitStack’s API, but subclasses could implement it (and name it whatever they want of course)?

I’m not sure _clone() is the right name here since that implies to me state copy as well, and I totally agree with your other points. That’s why I originally suggested _make_instance() would be the name and API in the base class.

[ncoghlan]

I'm not clear on what you mean about allowing arbitrary names for the instance creation function - at that point we're back to subclasses not being able to use the default pop_all() implementation at all, and needing to duplicate the state transfer logic in the subclass (which we don't provide a public API to do, since the _exit_callbacks queue is deliberately private).

However, I'm thinking we could potentially change pop_all itself to accept a target object:

    def pop_all(self, *, target=None):
        """Preserve the context stack by transferring it to a new instance

    If a *target* stack is given, it must be either an `ExitStack`
    instance, or else provide a callback registration method akin to `ExitStack.callback`.
    """
    if target is None:
        target = type(self)()
    exit_callbacks = self._exit_callbacks
    self._exit_callbacks = deque()
    if isinstance(target, ExitStack):
        target._exit_callbacks = exit_callbacks
    else:
        for cb in exit_callbacks:
            target.callback(cb)
    return target

The recipe fix would then be to make Callback.cancel() look like:

    def cancel(self):
        # We deliberately *don't* clean up the cancelled callback
        self.pop_all(target=ExitStack())

(Tangent: https://bugs.python.org/issue32445 suggests adding a small optimisation to ExitStack.callback to skip adding the wrapper when args and kwds are both empty)

[warsaw]

On Dec 29, 2017, at 08:40, Nick Coghlan <report@bugs.python.org> wrote:

I'm not clear on what you mean about allowing arbitrary names for the instance creation function -

What I meant was that I don’t see def _make_instance() defined in your example, so I didn’t know if that was part of the contract between the base and derived classes, or an “arbitrary method” that subclasses can come up with. From the example, it seems like the latter, but I could be misunderstanding your proposal.

at that point we're back to subclasses not being able to use the default pop_all() implementation at all, and needing to duplicate the state transfer logic in the subclass (which we don't provide a public API to do, since the _exit_callbacks queue is deliberately private).

However, I'm thinking we could potentially change pop_all itself to accept a target object:

def pop_all(self, *, target=None):
"""Preserve the context stack by transferring it to a new instance

   If a *target* stack is given, it must be either an `ExitStack`
   instance, or else provide a callback registration method akin to `ExitStack.callback`.
   """
   if target is None:
       target = type(self)()
   exit_callbacks = self._exit_callbacks
   self._exit_callbacks = deque()
   if isinstance(target, ExitStack):
       target._exit_callbacks = exit_callbacks
   else:
       for cb in exit_callbacks:
           target.callback(cb)
   return target

The recipe fix would then be to make Callback.cancel() look like:

def cancel(self):
# We deliberately don't clean up the cancelled callback
self.pop_all(target=ExitStack())

That’s actually not bad. I think I like that better than the (IMHO, misnamed) _clone() API. I’d quibble just a bit about the docstring, since the required API for target is exactly that it have a .callback() method that accepts a single exit callback argument.

[ncoghlan]

As per the comment at https://bugs.python.org/issue32445#msg309356, there's a bug in my suggested changes to ExitStack.pop_all(): the right method to call is ExitStack.push(), not ExitStack.callback() (the latter adds a wrapper function to make the signatures match, but our stored callbacks all already have the right signature).

I'm not too fussy about the details of the docstring, but we need to be careful about the guarantees we make to ExitStack subclasses: if the docstring implies that "target.push()" will always be called, then we need to limit the stack stealing behaviour to actual ExitStack instances (which may be a good idea anyway).