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.)

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.

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?

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.

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.

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)

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.

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).