The new context itertool is bad

[NeilGirdhar]

The new context itertool tries to expose a context manager as an iterable. This breaks the context manager manager guarantee that __exit__ will be called. It's not enough to tell the caller that he has to iterate over the whole iterable. Even if there are no break or return statements in the loop, there is always the possibility of exceptions. The whole point of context managers is to guarantee that the __exit__ is always called when a block terminates. This is why context managers and iterables are orthogonal concepts; in general, one cannot be made to look like the other.

Please remove context because it encourages people to write bad code.

There is no benefit to context in any case. Even the motivating example in the documentation is just:

consume(print(x, file=f) for f in context(file_obj) for x in it)

which can be written just as succinctly

with file_obj as f:
    consume(print(x, file=f) for x in it)

[bbayles]

See #112 and #114 for the discussions about this. I initially thought one should just write the with, but came around after that discussion.

Anyway, I'm not really sure about __exit__ not getting called. Given your issue description, I think you would expect Exit to not be printed, yes?

from more_itertools import consume, context

def iterable():
    yield 1
    raise RuntimeError('uh oh')
    yield 2

class Manager(object):
    def __enter__(self):
        print('Enter')
        return self
    
    def __exit__(self, *args, **kwargs):
        print('Exit')


manager = Manager()
it = iterable()
try:
    consume(print(x) for m in context(manager) for x in it)
except RuntimeError:
    print('Encountered exception!')

The output is in fact:

Enter
1
Encountered exception!
Exit

[bbayles]

Also CC @erikrose for comment. Should we remove this, or warn about gotchas in the docstring?

[NeilGirdhar]

The output is in fact:

Enter
1
Encountered exception!
Exit

This is a quirk of CPython, but is not promised by the Python language:

It is not guaranteed that __del__() methods are called for objects that still exist when the interpreter exits.

This code will not work in PyPy. If PyPy catches up to CPython down the line, all of the code that is written like this will break. (It was never supposed to work.)

Another problem is that the file handle may not be closed until the program exits, so a long-running program can easily run out of file handles or some other limited resource that was supposed to be relinquished at the end of the code block.

I read through the threads, but unfortunately, trying to roll context managers into iterators is problematic. You need a with construct when you want to do context management.

[erikrose]

I don't have a computer handy to confirm this, but I believe contextlib.contextmanager has the same problem, putting us within the already tolerated bounds of the stdlib. Sorry for brevity.

[erikrose]

Now I have a computer.

>>> from contextlib import contextmanager
>>> @contextmanager
... def foo():
...     print "enter"
...     yield
...     print "exit"
... 
>>> with foo():
...     raise NotImplementedError
... 
enter
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
NotImplementedError

There's no "exit", so I think we're okay. Maybe it's a design flaw in contextlib as well, but at least we're not doing something unprecedented. I could still hear it argued that exceptions are exceptional and that we promote skipping exit even if nothing goes wrong, but it's not immediately clear.

[NeilGirdhar]

If you want to use contextmanager and have cleanup code run despite exceptions, you need to wrap it:

@contextmanager
def NativePainting(qpainter):
    qpainter.beginNativePainting()
    try:
        yield
    finally:
        qpainter.endNativePainting()

[bbayles]

Most of the examples in the docs disagree?

@contextmanager
def tag(name):
    print("<%s>" % name)
    yield
    print("</%s>" % name)

You are correct that PyPy does not print the Exit in my example.

[bbayles]

For further context, the docs say:

If an unhandled exception occurs in the block, it is reraised inside the generator at the point where the yield occurred.
Thus, you can use a try...except...finally statement to trap the error (if any), or ensure that some cleanup takes place.
If an exception is trapped merely in order to log it or to perform some action (rather than to suppress it entirely), the generator must reraise that exception.
Otherwise the generator context manager will indicate to the with statement that the exception has been handled, and execution will resume with the statement immediately following the with statement.

The last sentence seems applicable to Erik's example with contextmanager, and probably ours here.

[NeilGirdhar]

Your quotation from the documentation says it all. If you're writing something that should happen even when there's an exception, then you have to wrap it as I did. If it's something unimportant like printing tags, it's probably ok to leave that off since the user probably has a stack dump to worry about.

Anyway, the reason I really don't like the context itertool is that it will encourage people to write what I consider to be bad code. This code is bad because it interacts poorly with flow control like exceptions, break statements, and return statements. Also this code might potentially one day interfere with PyPy's adoption.

There was a proposal actually to unify iteration and context managers. The motivation is very similar as that of this itertool, but their solution was to propose a langauge change. There's a lot interesting reading on this topic in that PEP and the discussion surrounding that PEP.

[NeilGirdhar]

To highlight why I think that quotation supports not keeping the context itertool:

Thus, you can use a try...except...finally statement to trap the error (if any), or ensure that some cleanup takes place.

If someone does this in their context manager and then you use it with the context itertool, the cleanup that they think will take place, won't.

[bbayles]

I hate to zap a thing after just releasing it, but after playing around with PyPy I'm convinced that Neil is right and that there's not a good way to warn about this in the documentation.

I'm inclined to remove context(), make a note in the version history, and bring back #115?

[bbayles]

I'll let Erik comment before I pull the trigger on this, but here's my plan:

• Add PyPy (and PyPy3) to the Travis environments - we would have noticed this issue earlier if we'd had them; tests against master fail on PyPy.
• Remove context() and release a new major version (3.0.0) because of the API change
• Put a note about proper context management for anyone who was using context() before in the release notes / version history
• Add back in the additions to side_effect() that we discussed as an alternative to context() originally (these have no problem on PyPy, and I even had a test for the exceptional case)

All that is in this branch.

[erikrose]

Sounds sane to me.

[bbayles]

All fixed. Thanks Neil for the explanation.