[issue13585] Add contextlib.CleanupManager

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor

New submission from Nikolaus Rath <[hidden email]>:

I'd like to propose addding the CleanupManager class described in http://article.gmane.org/gmane.comp.python.ideas/12447 to the contextlib module. The idea is to add a general-purpose context manager to manage (python or non-python) resources that don't come with their own context manager.

Example code:

with CleanupManager() als mngr:
    tmpdir = tempfile.mkdtemp()
    mngr.register(shutil.rmtree(tmpdir))
    # do stuff with tmpdir

# shutil.rmtree() get's called automatically when the block is over

Note that mkdtemp() could of course also be changed to become its own context manager. The idea is to provide a general facility for this kind of problem, so it doesn't have to be reinvented whenever a module provides a ressource without its own context manager. Other possible uses are of course ressources that are completely external to Python,
e.g. anything allocated with a subprocess (think of subprocess.check_call('mount'))/


I'll be happy to make a proper patch with documentation and testcases from Jan's code. As a matter of fact, I'll probably start working out it right now, so please let me know quickly if this doesn't have a chance of getting accepted.

----------
components: Library (Lib)
messages: 149268
nosy: Nikratio
priority: normal
severity: normal
status: open
title: Add contextlib.CleanupManager
type: feature request
versions: Python 3.3, Python 3.4

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor

Nikolaus Rath <[hidden email]> added the comment:

Here's the first part of the patch with the implementation. I'll add tests and documentation as soon as someone confirms that the idea & API is okay.

----------
keywords: +patch
Added file: http://bugs.python.org/file23923/CleanupManager_patch_v1.diff

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Raymond Hettinger <[hidden email]> added the comment:

I would like to see this posted as a recipe before being put in the standard library.  It needs a chance to mature and to demonstrate that people will want to use it.

FWIW, the example is problematic in a couple of ways.  The registration of shutil.rmtree(tmpdir) will run *before* mngr register is called.  

Also, it doesn't take advantage of any of the with-statement features.  It doesn't show any advantage over a standard try/finally which is arguably cleaner:

    tmpdir = tempfile.mkdtemp()
    try:
        # do stuff with tmpdir
    finally:
        shutil.rmtree()

Also, I suspect that the CleanupManager would be an error-prone construct because the registration occurs somewhere after the with-statement is set-up, possibly resulting in errors if there is an early, pre-registration failure.

----------
nosy: +rhettinger

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Nikolaus Rath <[hidden email]> added the comment:

Not sure what you mean with "posted as a recipe" -- are you thinking of a specific website/mailing list?

Example: which one do you mean? The one in the issue or the one in the patch?

With statement: what advantages do you have in mind?

Try/finally: I think the patch and the discussion in python-ideas talk about the advantage over try/finally. IMO the two most important points are: (1) avoids deep and pointless indendation for multiple ressources, (2) keeps logically connected lines (allocation+cleanup) closely together in the source instead of splitting them far apart like try/finally.

error-prone: not sure if I understand you correctly. If there is an error prior to registration, the callback will not be called (that's a feature). To what kind of errors could that lead?


Sorry for basically asking you to re-explain every sentence, but I honestly don't understand most of your message.

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Raymond Hettinger <[hidden email]> added the comment:

'''
Example code:

with CleanupManager() als mngr:
    tmpdir = tempfile.mkdtemp()
    mngr.register(shutil.rmtree(tmpdir)) <-- this makes the call right away
    # do stuff with tmpdir
'''

The part of my note that should be clear is that the idea and code need to prove itself before being added to the standard library.  So far, there has been zero demand for this and I've not seen code like it being used in the wild.  AFAICT, it is not demonstrably better than a straight-forward try/finally.

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor

Nikolaus Rath <[hidden email]> added the comment:

On 12/11/2011 10:17 PM, Raymond Hettinger wrote:

>
> Raymond Hettinger <[hidden email]> added the comment:
>
> '''
> Example code:
>
> with CleanupManager() als mngr:
>     tmpdir = tempfile.mkdtemp()
>     mngr.register(shutil.rmtree(tmpdir)) <-- this makes the call right away
>     # do stuff with tmpdir
> '''

Oh, of course. That is fixed in the patch. I couldn't find a way to edit
the message in the tracker.

> The part of my note that should be clear is that the idea and code need to prove itself before being added to the standard library.  So far, there has been zero demand for this and I've not seen code like it being used in the wild.  AFAICT, it is not demonstrably better than a straight-forward try/finally.

I think it has the same advantages over try...finally as any use of
'with' has. CleanupManager would allow to use 'with' instead of
'try..finally' in many cases where this is currently not possible, so
unless the introduction of 'with' itself was a mistake, I think this is
just taking a step along the path that has already been chosen.

Best,

   -Nikolaus

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Changes by Nikolaus Rath <[hidden email]>:


Added file: http://bugs.python.org/file23933/CleanupManager_patch_v2.diff

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Changes by Giampaolo Rodola' <[hidden email]>:


----------
nosy: +giampaolo.rodola

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Sven Marnach <[hidden email]> added the comment:

There is actually a second thread on python-ideas on a very similar topic, see

    http://mail.python.org/pipermail/python-ideas/2011-December/013021.html

The two main advantages of the proposed CleanupManager over try/finally blocks are

1. You can add a clean-up function conditionally.  In a try/finally block, you would need a flag, which would scatter the single idea more across the code.  Example:

    with CleanupManager() als mngr:
        if f is None:
            f = open("some_file")
            mngr.register(f.close)
        # do something with f (possibly many lines of code)

seems much clearer to me than

    f_needs_closing = False
    if f is None:
        f = open("some_file")
        f_needs_closing = True
    try:
        # do something with f (possibly many lines of code)
    finally:
        if f_needs_closing:
            f.close()

The first version is also much more in the spirit of context managers.  You state at the beginning "we open the file, and guarantee that it will be closed", and we know right from the start that we don't have to bother with it again.

2. CleanupManager could replace several nested try/finally blocks, which again might lead to more readable code.

On the other hand, people who never saw ContextManager before will have to look it up, which will impair readability for those people.

Adding this as a cookbook recipe first seems like a good idea.

----------
nosy: +smarnach

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor

Nikolaus Rath <[hidden email]> added the comment:

On 12/12/2011 03:31 PM, Sven Marnach wrote:
> Adding this as a cookbook recipe first seems like a good idea.

This is the second time that this is mentioned, so I would be very
grateful if someone could elucidate what "adding as a cookbook recipe"
means :-). Is there an official Python cookbook somewhere?

Best,

   -Nikolaus

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Eric Snow <[hidden email]> added the comment:

Check out: http://code.activestate.com/recipes/

----------
nosy: +eric.snow

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Julian Berman <[hidden email]> added the comment:

For reference, the implementation that I posted in the other thread is:

    @contextlib.contextmanager
    def maybe(got, contextfactory, *args, checkif=bool, **kwargs):
        if checkif(got):
            yield got
        else:
            with contextfactory(*args, **kwargs) as got:
                yield got

which produces code like:

def fn(input_file=None):
    with maybe(input_file, open, "/default/input/file/location/"):
        for line in input_file:
            print line

For the example given here in the OP, since rmtree isn't a contextmanager it'd require wrapping it in one first.

I could probably understand if there was desire for these to be a recipe instead. The advantage (if any) of the above over this class is that it keeps context managers sorta looking like context managers. Here if you wanted to write my example it'd require writing code like

with ContextManager() as mgr:
    foo = ctxtmgr()
    mgr.register(foo.__close__)

which doesn't look too great :/

The class though does have wider scope though, since it doesn't necessarily only need a context manager, it can do cleanup for anything, which I guess is nice.

----------
nosy: +Julian

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Nick Coghlan <[hidden email]> added the comment:

Given the existence of tempfile.TemporaryDirectory in recent Python versions, I suggest finding a new cleanup function example that doesn't duplicate native stdlib functionality :)

I do see value in the feature itself though - I believe the precedent of both the atexit module [1] and unittest.TestCase.addCleanup() [2] shows how it can be useful to have a standard way to accumulate a sequence of "undo" operations for invocation at a later time.

In particular, it's a much better fit than nested with statements are for *optional* resources - you can make the with statement unconditional (setting up the cleanup manager), optionally add the cleanup methods, and avoid needing to have two copies of your actual invocation code (one inside a with statement and one without) or having a delayed check in a finally block.

It codifies the fairly common "if this resource was acquired, make sure it is released" idiom in a way that with statements and try/finally just don't handle neatly. By using an incremental API, it also avoids the traps associated with the ultimately misguided "contextlib.nested()" design.

However, I suggest using an API that strictly follows the "register only" model employed by both of the existing mechanisms. In addition, any solution provided as part of contextlib should interoperate nicely with existing context managers - it should be trivial to rewrite a nested with statement to be based on CleanupManager instead.

Accordingly, I would give the manager the following public methods:

  register_exit() (only accepts callbacks with the __exit__ signature)
  register() (equivalent to TestCase.addCleanup)
  enter_context() (accepts actual context managers)
  close() (equivalent to TestCase.doCleanups)

register_exit() would be the base callback registration method. It would accept only callbacks with the same signature as __exit__ methods.

    def register_exit(self, exit):
        self._callbacks.append(exit)
        return exit # Allow use as a decorator

register() would wrap arbitrary callbacks to support the __exit__ method signature:

    def register(self, _cb, *args, **kwds):
        def _wrapper(exc_type, exc, tb):
            return _cb(*args, **kwds)
        return self.register_exit(_wrapper)

enter_context() would work as follows:

    def enter_context(self, cm):
        result = cm.__enter__()
        self.register_exit(cm.__exit__)
        return result

close() would look like:

    def close(self):
        self.__exit__(None, None, None)

And finally, __exit__() itself would be:

    def __exit__(self, *exc_details):
        def _invoke_next_callback(exc_details):
            # Callbacks are removed from the list in FIFO order
            # but are actually *invoked* in LIFO order
            cb = self._callbacks.pop(0)
            if not self._callbacks:
                # Innermost callback is invoked directly
                return cb(exc_type, exc, tb)
            # Use try-finally to ensure this callback still gets
            # invoked even if an inner one fails
            try:
                inner_result = _invoke_next_callback()
            except:
                cb_result = cb(*sys.exc_info())
                # Check if this cb suppressed the inner exception
                if not cb_result:
                    raise
            else:
                # Check if inner cb suppressed the original exception
                if inner_result:
                    exc_details = (None, None, None)
                cb_result = cb(*exc_details)
            return cb_result
        _invoke_next_callback(exc_details)

An example using a cleanup manager to handle multiple files, one of which is optional:

    with contextlib.CleanupManager() as cm:
        source = cm.enter_context(open(source_fname))
        if dest_fname is not None:
            dest = cm.enter_context(open(dest_fname))
            _write_to_dest = dest.write
        else:
            def _write_to_dest(line): pass
        for line in source:
            _write_to_dest(line)
            yield line


[1] http://docs.python.org/library/atexit
[2] http://docs.python.org/library/unittest#unittest.TestCase.addCleanup

----------
nosy: +ncoghlan

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Raymond Hettinger <[hidden email]> added the comment:

I think you guys need to post your code somewhere (perhaps on PyPi or the ASPN Cookbook).  It needs to mature beyond the stage of "I just whipped-up this code and think it would be great if everybody used it".

I've seen nothing like this being used in production code (code published on the net or at one of my clients).  Context managers have been around for a while, so if this were a real need, we would expect to see people already using something like this (for example, namedtuples got introduced to the standard library upon seeing many, many reinventions of the concept and we were able to consolidate the best features from each).

Design of the a feature in the standard library should be driven by examples of real world code that would be improved with the new feature.  The design should also be informed by the experience of teaching people how to use it and seeing what they do (lots of technically correct ideas get shot down because it turns out that incorrect usage is common (a bug factory like the % formatting operator) or that people have a hard time learning and remembering the feature.

The core problem is that it is easier to add things to the standard library than to take them out if they prove to be a bad idea.  Accordingly, we need to be *really sure* that this is a good idea, that it will *improve* real world code, that people learn, understand, and remember it easily, and that is doesn't impair readability.

The example code from Nikolaus has a bug in it -- that is worrisome and may suggest that we are better-off without this being in the standard library.

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Changes by Raymond Hettinger <[hidden email]>:


----------
assignee:  -> rhettinger
resolution:  -> later

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Nick Coghlan <[hidden email]> added the comment:

TestCase.setUp() and TestCase.tearDown() were amongst the precursors to__enter__() and __exit__(). addCleanUp() fills exactly the same role here - and I've seen *plenty* of positive feedback directed towards Michael for that addition to the unittest API.

For individual one-off cases, a flag variable and an if statement inside a finally block is an adequate, but not ideal, solution, because it suffers from all the readability and auditability problems of *any* try/finally based solution. It's particularly annoying when an object *does* support the context management protocol, but I can't use a with statement simply because I don't *always* need (and/or own) that resource (this kind of thing happens in a few places in runpy, since the behaviour changes depending on whether or not runpy created temporary objects for itself or was given objects as arguments)

Custom context managers are typically a bad idea in these circumstances, because they make readability *worse* (relying on people to understand what the context manager does). A standard library based solution, on the other hand, offers the best of both worlds:
- code becomes easier to write correctly and to audit for correctness (for all the reasons with statements were added in the first place)
- the idiom will eventually become familiar to all Python users

If other "cleanup function" registration APIs didn't already exist, I'd agree with you that this needed further exposure. However, I simply don't agree that's the case - atexit and addCleanup provide your field testing, the rest of the design is just a matter of integrating those concepts with the context management protocol.

Indeed, one of the objections I received after we deprecated contextlib.nested() was that you couldn't easily pass a programmatically generated list of resources to nested with statements. Given contextlib.CleanupManager it becomes trivial:

    with contextlib.CleanupManager as cm:
        files = [cm.enter_context(open(fname)) for fname in names]
        # All files will be closed when we leave the context

I can take this up on python-dev if you want, but I hope to persuade you that the desire *is* there, it's just that the workarounds for the lack of this functionality involve avoiding the context management protocol entirely:

    try:
        files = [open(fname) for fname in names]
        # Are all files closed when we're done?
        # I dunno, scroll down past the algorithm code to check!


        # Avoiding this would be good for all the reasons the
        # with statement was added in the first place

    finally:
        for f in files:
            f.close()

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Nick Coghlan <[hidden email]> added the comment:

Given the history of API design errors in contextlib (cf. contextlib.nested in general, making contextlib._GeneratorContextManager a subclass of contextlib.ContextDecorator), I've realised Raymond is right in wanting to see this idea more thoroughly vetted before it gets added to the standard library.

Accordingly, I plan to create a 'unittest2' style backport library for contextlib (imaginatively named 'contextlib2') and prototype the concept further there.

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Nick Coghlan <[hidden email]> added the comment:

In the meantime, I put my version up as a cookbook recipe: http://code.activestate.com/recipes/577981-cleanupmanager-for-with-statements/

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Raymond Hettinger <[hidden email]> added the comment:

Thanks Nick.  You're awesome.

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

Reply | Threaded
Open this post in threaded view
|

[issue13585] Add contextlib.CleanupManager

STINNER Victor
In reply to this post by STINNER Victor

Nick Coghlan <[hidden email]> added the comment:

And the backport: http://contextlib2.readthedocs.org/

I haven't tested on anything other than 2.7 as yet - I have an account request in train with the Shining Panda folks, so I'll set up multi-version CI for this project (along with a couple of others) once that goes through.

----------

_______________________________________
Python tracker <[hidden email]>
<http://bugs.python.org/issue13585>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/lists%2B1322467933539-512619%40n6.nabble.com

12