classification
Title: Attempting to use class with both __enter__ & __exit__ undefined yields __exit__ attribute error
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: Spencer Brown, benjamin.peterson, ellingtonjp, maggyero, ncoghlan, ned.deily, python-dev, rhettinger, serhiy.storchaka, xiang.zhang, yselivanov
Priority: normal Keywords: patch

Created on 2016-05-24 03:28 by ellingtonjp, last changed 2019-12-14 20:02 by maggyero. This issue is now closed.

Files
File name Uploaded Description Edit
with.patch ellingtonjp, 2016-05-24 03:28 review
fix_with_refleak.diff rhettinger, 2016-11-22 18:20 Fix reference leak
regexp-deprecated.patch martin.panter, 2016-11-24 04:13
Pull Requests
URL Status Linked Edit
PR 17609 open maggyero, 2019-12-14 19:58
Messages (24)
msg266219 - (view) Author: Jonathan Ellington (ellingtonjp) * Date: 2016-05-24 03:28
Attempting to use class that has both __exit__() and __enter__() undefined as a context manager yields an AttributeError referring to __exit__:

>>> class A():
...     pass
...
>>> with A():
...     pass
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: A instance has no attribute '__exit__'


Knowing that the 'with' statement should first execute __enter__, this is unexpected.

The patch ensures the attribute error refers to __enter__() when both methods are undefined.
msg266222 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-05-24 03:55
This seems to be designed. From PEP343 it tells clearly:

    If either of the relevant methods are not found
    as expected, the interpreter will raise AttributeError, in the
    order that they are tried (__exit__, __enter__).

But currently I don't find out why.
msg266226 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-05-24 05:28
Nick, I've seen the come-up several times with people learning about context managers.  The current error message seems cause confusion because it checks for __exit__ before __enter__, implying that the __enter__ is optional which it isn't.   Do you have any objections testing for __enter__ first?
msg266227 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2016-05-24 05:41
I don't completely understand. If you get an AttributeError for __enter__, will you think only an __enter__ is required?
msg266232 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-05-24 06:10
As Jonathan notes, the cause for confusion here is that we currently retrieve the attributes in the opposite order from the way we execute them, prompting the "Why is it looking for __exit__ first?", "Oh, __enter__ must be optional, so it didn't care that it was missing" train of thought.

If I recall correctly, rather than stemming from any particular design principle, the current behaviour is an artifact of the initial implementation that didn't use any dedicated opcodes: since CPython uses a stack-based VM, we needed __enter__ to be above __exit__ on the stack, and the most efficient way to achieve that was to do the attribute lookups in the reverse order of invocation.

When SETUP_WITH was introduced, that original method lookup ordering (and associated error message precedence) was preserved, even though the technical rationale for that ordering no longer applied.

Given that, I don't see any problem with switching the lookup order now, and I agree that should be less confusing, given that the most obvious explanation for getting an error message about __enter__ when both are missing is that the interpreter tries to call __enter__ before it tries to do anything with __exit__. (That intuitive explanation still isn't 100% accurate, but it's closer than thinking __enter__ is optional)
msg277570 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-09-28 00:54
> I don't see any problem with switching the lookup order now,
> and I agree that should be less confusing,

Thanks Nick.  I concur as well and will apply this.

Would it be reasonable to apply this to 3.6?  IMO, it isn't a new feature, it is more of a usability bug due to an oversight.
msg277592 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-28 06:50
+1 for 3.6 from me - as you say, this is fixing a usability bug in the error handling rather than being a new feature.
msg277666 - (view) Author: Spencer Brown (Spencer Brown) * Date: 2016-09-28 21:45
Alternatively, SETUP_WITH could additionally check for the other method, and raise an AttributeError with a custom message mentioning both methods.
msg277735 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-30 03:07
Due to the expected semantics of AttributeError being relatively precise, changing to a protocol check rather than just changing the order we look up the protocol attributes would also involving changing from AttributeError to TypeError.

Since that's a potentially more disruptive change and just changing the order of the checks is likely sufficient to reduce confusion, the latter approach is preferable, at least for now (if folks are still getting confused by the time 3.7 rolls around, then that option would be worth reconsidering).
msg281426 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-11-22 01:25
New changeset 3aafb232f2db by Raymond Hettinger in branch '3.6':
Issue #27100:  With statement reports missing __enter__ before __exit__.  (Contributed by Jonathan Ellington.)
https://hg.python.org/cpython/rev/3aafb232f2db
msg281427 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-11-22 01:26
Thanks for the patch Jonathan.  Nick, thanks for chiming in.
msg281454 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-11-22 09:28
https://mail.python.org/pipermail/python-checkins/2016-November/147171.html

test_collections leaked [0, 0, 7] memory blocks, sum=7
test_contextlib leaked [38, 38, 38] references, sum=114
test_contextlib leaked [13, 12, 12] memory blocks, sum=37
test_descr leaked [164, 164, 164] references, sum=492
test_descr leaked [59, 59, 59] memory blocks, sum=177
test_functools leaked [0, 3, 1] memory blocks, sum=4
test_with leaked [37, 37, 37] references, sum=111
test_with leaked [13, 13, 13] memory blocks, sum=39

Command line was: ['./python', '-m', 'test.regrtest', '-uall', '-R', '3:3:/home/psf-users/antoine/refleaks/reflogWMjqsJ', '--timeout', '7200']

Looking at the code change, it looks like you aren’t releasing __enter__ if the __exit__ lookup fails.

A separate question: why not swap the “async with” __aenter__ and __aexit__ code at the same time?
msg281455 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-11-22 09:31
Hmm, it seems I already discovered this leak six months ago: https://bugs.python.org/review/27100
msg281497 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-11-22 18:23
Serhiy, do you care to double check this patch?

Martin, I'll leave the async_with for Yuri to fix-up.  I believe that is his code, so he should do the honors.
msg281505 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-22 19:43
The patch LGTM.
msg281509 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-11-22 19:51
New changeset 749c5d6c4ba5 by Raymond Hettinger in branch '3.6':
Issue #27100:  Fix ref leak
https://hg.python.org/cpython/rev/749c5d6c4ba5
msg281510 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-11-22 19:53
Yuri, do you want to do the same for async-with?
msg281602 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-11-24 04:13
The tests changes also produce a DeprecationWarning:

======================================================================
ERROR: testEnterAttributeError1 (test.test_with.FailureTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/proj/python/cpython/Lib/test/test_with.py", line 120, in testEnterAttributeError1
    self.assertRaisesRegexp(AttributeError, '__enter__', fooLacksEnter)
  File "/home/proj/python/cpython/Lib/unittest/case.py", line 1311, in deprecated_func
    DeprecationWarning, 2)
DeprecationWarning: Please use assertRaisesRegex instead.

The fix is simple and just affects the tests, but since most people don’t seem to worry about warnings, I guess this can wait for 3.6 to be released before committing the fix.
msg281606 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-24 05:45
Oh, how I missed this? I usually run tests with -Wa.

Since the patch is simple and fixes a regression introduced just before releasing beta 4, I'm fine with fixing it. We are trying to support tests warnings-free. Some buildbots can run tests with -We.
msg281610 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-11-24 06:55
> Since the patch is simple and fixes a regression introduced
> just before releasing beta 4, I'm fine with fixing it. We are
> trying to support tests warnings-free. Some buildbots can run 
> tests with -We.

+1
msg281643 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-11-24 18:51
New changeset e11df6aa5bf1 by Raymond Hettinger in branch '3.6':
Issue #27100:  Silence deprecation warning in Lib/test/test_with.py
https://hg.python.org/cpython/rev/e11df6aa5bf1
msg291092 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-04-03 18:58
Can this be closed now?
msg291124 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-04-04 15:13
Reviewing the discussion, I assume this was left open to cover reordering the __aenter__ and __aexit__ checks for async with, but that can just as easily be handled as a separate issue (which would also be clearer at the NEWS level).
msg358404 - (view) Author: Géry (maggyero) * Date: 2019-12-14 20:02
@ncoghlan

> Reviewing the discussion, I assume this was left open to cover reordering the __aenter__ and __aexit__ checks for async with, but that can just as easily be handled as a separate issue (which would also be clearer at the NEWS level).

Here it is: https://bugs.python.org/issue39048
History
Date User Action Args
2019-12-14 20:02:46maggyerosetmessages: + msg358404
2019-12-14 19:58:53maggyerosetpull_requests: + pull_request17081
2019-12-14 19:57:59maggyerosetpull_requests: - pull_request17079
2019-12-14 19:47:38maggyerosetpull_requests: + pull_request17079
2019-12-13 16:08:17maggyerosetnosy: + maggyero
2017-04-04 15:13:02ncoghlansetstatus: open -> closed

messages: + msg291124
stage: patch review -> resolved
2017-04-03 18:58:10rhettingersetmessages: + msg291092
2017-04-01 06:26:51martin.pantersetnosy: - martin.panter
2017-04-01 05:49:19serhiy.storchakasetpull_requests: - pull_request1060
2017-03-31 16:36:33dstufftsetpull_requests: + pull_request1060
2016-11-24 18:51:06python-devsetmessages: + msg281643
2016-11-24 06:55:25rhettingersetmessages: + msg281610
2016-11-24 05:45:32serhiy.storchakasetmessages: + msg281606
2016-11-24 04:13:00martin.pantersetfiles: + regexp-deprecated.patch

messages: + msg281602
2016-11-22 19:53:22rhettingersetpriority: release blocker -> normal
assignee: rhettinger -> yselivanov
messages: + msg281510
2016-11-22 19:51:07python-devsetmessages: + msg281509
2016-11-22 19:43:30serhiy.storchakasetassignee: serhiy.storchaka -> rhettinger
messages: + msg281505
2016-11-22 18:23:30rhettingersetassignee: rhettinger -> serhiy.storchaka

messages: + msg281497
nosy: + serhiy.storchaka, yselivanov
2016-11-22 18:20:28rhettingersetmessages: - msg281495
2016-11-22 18:20:17rhettingersetfiles: + fix_with_refleak.diff
2016-11-22 17:56:25rhettingersetmessages: + msg281495
2016-11-22 17:42:12serhiy.storchakasetpriority: normal -> release blocker
2016-11-22 09:31:42martin.pantersetmessages: + msg281455
2016-11-22 09:28:13martin.pantersetstatus: closed -> open
nosy: + martin.panter
messages: + msg281454

2016-11-22 01:26:13rhettingersetstatus: open -> closed
resolution: fixed
messages: + msg281427
2016-11-22 01:25:17python-devsetnosy: + python-dev
messages: + msg281426
2016-09-30 03:07:42ncoghlansetmessages: + msg277735
2016-09-28 21:45:27Spencer Brownsetnosy: + Spencer Brown
messages: + msg277666
2016-09-28 19:16:11ned.deilysetstage: patch review
versions: + Python 3.6
2016-09-28 06:50:52ncoghlansetmessages: + msg277592
2016-09-28 00:54:43rhettingersetnosy: + ned.deily
messages: + msg277570
2016-09-27 12:42:48serhiy.storchakasetversions: + Python 3.7, - Python 3.6
2016-05-24 06:10:23ncoghlansetmessages: + msg266232
2016-05-24 05:41:14benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg266227
2016-05-24 05:28:48rhettingersetversions: + Python 3.6
nosy: + ncoghlan

messages: + msg266226

assignee: rhettinger
2016-05-24 03:55:37xiang.zhangsetnosy: + xiang.zhang
messages: + msg266222
2016-05-24 03:28:37ellingtonjpcreate