[issue32145] Wrong ExitStack Callback recipe

[Nick Coghlan]

Nick Coghlan  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Barry A. Warsaw]

Barry A. Warsaw  added the comment:

On Dec 29, 2017, at 08:40, Nick Coghlan  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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Nick Coghlan]

Nick Coghlan  added the comment:

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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Barry A. Warsaw]

Barry A. Warsaw  added the comment:

On Dec 25, 2017, at 18:51, Nick Coghlan  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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Barry A. Warsaw]

Barry A. Warsaw  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--
nosy: +barry
versions: +Python 3.7 -Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Berker Peksag]

Change by Berker Peksag :


--
nosy: +ncoghlan
type: crash -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32145] Wrong ExitStack Callback recipe

[Maurizio Zucchelli]

New submission from Maurizio Zucchelli :

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

--
assignee: docs@python
components: Documentation
messages: 307037
nosy: Denaun, docs@python
priority: normal
severity: normal
status: open
title: Wrong ExitStack Callback recipe
type: crash
versions: Python 3.4, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com