bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeE… #891

soolabettu · 2017-03-30T00:56:33Z

…rror

mention-bot · 2017-03-30T00:56:35Z

@svelankar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @brettcannon and @Yhg1s to be potential reviewers.

soolabettu · 2017-03-30T00:56:47Z

Please review.

JelleZijlstra · 2017-03-30T01:00:22Z

Can you add a test?

soolabettu · 2017-03-30T02:04:35Z

ok

…rror Committer: Siddharth Velankar <[email protected]>

soolabettu · 2017-03-30T09:16:32Z

Made changes as per comments from serhiy.storchaka . Havent added an explicit return at the end of the function as it becomes unreachable after my changes, i think.

serhiy-storchaka

Needed a test.

serhiy-storchaka · 2017-03-30T17:07:33Z

Lib/contextlib.py

-                return exc is not value
+                # Conform to PEP 8 - "Either all return statements in a function
+                # should return an expression, or none of them should".
+                if exc is not value:


This doesn't look as an enhancement to me. No need to change this line.

The added comment is also incorrect, as exc is not value is already an expression.

serhiy-storchaka · 2017-03-30T17:12:55Z

Lib/contextlib.py

                # fixes the impedance mismatch between the throw() protocol
                # and the __exit__() protocol.
                #
                if sys.exc_info()[1] is not value:


@ncoghlan, is it worth to negate the condition and swap raise and return for uniformity with the above code? Shouldn't the function return False instead of None for uniformity?

brettcannon · 2017-03-30T21:45:59Z

Lib/contextlib.py

                next(self.gen)
            except StopIteration:
-                return
+                return None


This isn't necessary as a bare return implicitly returns None.

For consistency with the other return statements, this should be return False.

brettcannon · 2017-03-30T21:46:11Z

Lib/contextlib.py

                #
                if sys.exc_info()[1] is not value:
                    raise
+                return None


This is not necessary as all functions and methods implicitly has a return statement at the end of them.

I like @serhiy-storchaka's suggestion of making this more consistent with the except RuntimeError case, and making this clause be:

# Use sys.exc_info() for consistency with the contextlib2 backport if sys.exc_info()[1] is value: return False raise

That way all the exception handling clauses are either return <boolean expression> (if suppressing the exception is one of the available options), or else a series of conditional return False statements followed by an unconditional raise.

soolabettu · 2017-03-30T23:59:24Z

@brettcannon , please have a look at issue #29692 for a comment regarding conformance to PEP 8 and my subsequent changes to adhere to it, thanks.

brettcannon · 2017-03-31T00:02:36Z

@svelankar fair enough!

ncoghlan

The main missing piece is a test case to show that the original error no longer happens, but I have a few other minor comments inline.

ncoghlan · 2017-03-31T02:16:26Z

Lib/contextlib.py

-                return exc is not value
+                # Conform to PEP 8 - "Either all return statements in a function
+                # should return an expression, or none of them should".
+                if exc is not value:


The added comment is also incorrect, as exc is not value is already an expression.

ncoghlan · 2017-03-31T02:22:57Z

Lib/contextlib.py

                # was passed to throw() and later wrapped into a RuntimeError
                # (see PEP 479).
-                if exc.__cause__ is value:
+                if exc.__cause__ is value and type is StopIteration:


To match the order of the description in the comment, I suggest checking the passed in exception type first:

if type is StopIteration and exc.__cause__ is value: return False

ncoghlan · 2017-03-31T02:29:38Z

Lib/contextlib.py

                next(self.gen)
            except StopIteration:
-                return
+                return None


For consistency with the other return statements, this should be return False.

ncoghlan · 2017-03-31T02:42:08Z

Lib/contextlib.py

                #
                if sys.exc_info()[1] is not value:
                    raise
+                return None


I like @serhiy-storchaka's suggestion of making this more consistent with the except RuntimeError case, and making this clause be:

# Use sys.exc_info() for consistency with the contextlib2 backport if sys.exc_info()[1] is value: return False raise

That way all the exception handling clauses are either return <boolean expression> (if suppressing the exception is one of the available options), or else a series of conditional return False statements followed by an unconditional raise.

ncoghlan · 2017-03-31T02:44:20Z

Lib/contextlib.py

                    raise
+                return None
            else:
                raise RuntimeError("generator didn't stop after throw()")


Given @serhiy-storchaka's comments about readability, I'm wondering if it may make sense to drop some of the else: clauses in this function, such that this final raise ends up clearly being the final unconditionally executed line in the function. At the moment, it's far from clear that every path of execution ends in either return or raise, so it isn't actually possible to "fall off the end" and return None.

soolabettu · 2017-03-31T11:31:15Z

Will add test case shortly

#505) unlucky Unicode characters.

…#502) os.fwalk() is sped up by 2 times by using os.scandir().

if pass `accept={int, NoneType}`.

…Error. (#680) ValueError always is raised rather than OverflowError for negative counts. Shifting zero with non-negative count always returns zero.

* test_normalization fails if download fails bpo-29887. The test is still skipped if "-u urlfetch" option is not passed to regrtest (python3 -m test -u urlfetch test_normalization). * Fix ResourceWarning in test_normalization bpo-29887: Fix ResourceWarning in test_normalization if tests are interrupted by CTRL+c.

…#773) Element.getiterator() and the html parameter of XMLParser() were deprecated only in the documentation (since Python 3.2 and 3.4 correspondintly). Now using them emits a deprecation warning. * Don’t need check_warnings any more.

…and deque (#887) when pass indices of wrong type.

…#889) Fix the use of recursion in itertools.chain.from_iterable. Using recursion is unnecessary, and can easily cause stack overflows, especially when building in low optimization modes or with Py_DEBUG enabled.

…n urllib.request module. (#918)

s/keys and elements/keys and values/

Changed test code to suppress a compiler warning, while taking care to avoid the code being optimized out by the compiler.

Make a non-Py_DEBUG, asserts-enabled build of CPython possible. This means making sure helper functions are defined when NDEBUG is not defined, not just when Py_DEBUG is defined. Also fix a division-by-zero in obmalloc.c that went unnoticed because in Py_DEBUG mode, elsize is never zero.

Fix MemoryError caused by moving around code in PR #886; nbytes was sometimes used unitinitalized (in non-debug builds, when use_calloc was false and elsize was 0).

…ime (#927) objects when pass out of bound fold argument.

Also uncomment and fix a path test.

…v6. (#879) the original logic was just comparing the network address but this is wrong because if the network address is equal then we need to compare the ip address for breaking the tie add more ip_interface comparison tests

This reverts commit b94d7fd.

Revert "Minor factoring: move redundant resize scaling logic into the resize function." This reverts commit 4897300.

…in RuntimeError

soolabettu · 2017-04-01T12:07:25Z

Implemented review comments and added test case - please review.

bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeE…

cd8eaf8

…rror

the-knights-who-say-ni added the CLA signed label Mar 30, 2017

bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeE…

ac33e3d

…rror Committer: Siddharth Velankar <[email protected]>

serhiy-storchaka requested review from 1st1 and ncoghlan March 30, 2017 17:05

serhiy-storchaka requested changes Mar 30, 2017

View reviewed changes

brettcannon reviewed Mar 30, 2017

View reviewed changes

brettcannon added the type-bug An unexpected behavior, bug, or error label Mar 30, 2017

ncoghlan requested changes Mar 31, 2017

View reviewed changes

orsenthil and others added 12 commits April 1, 2017 08:01

Remove an unrequired TODO in test_urllib2. (#897)

7fddc7a

bpo-29878: Add global instances of int for 0 and 1. (#852)

4032e0a

bpo-24821: Fixed the slowing down to 25 times in the searching of some (

c0ba6c3

#505) unlucky Unicode characters.

bpo-25996: Added support of file descriptors in os.scandir() on Unix. (…

cc08f5d

…#502) os.fwalk() is sped up by 2 times by using os.scandir().

bpo-29852: Argument Clinic Py_ssize_t converter now supports None (#716)

e5961b9

if pass `accept={int, NoneType}`.

bpo-29816: Shift operation now has less opportunity to raise Overflow…

fdfe71b

…Error. (#680) ValueError always is raised rather than OverflowError for negative counts. Shifting zero with non-negative count always returns zero.

bpo-27863: Fixed multiple crashes in ElementTree. (#765)

938a17d

bpo-29918: Add missed "const" modifiers in C API documentation. (#846)

9ab918d

bpo-29913: deprecate compare_networks() in documentation (GH-865)

c4c1b2b

bpo-20548: Use specific asserts in warnings and exceptions tests (#788)

aa6a3bd

serhiy-storchaka and others added 18 commits April 1, 2017 08:01

bpo-29935: Fixed error messages in the index() method of tuple, list …

f8a4fe9

…and deque (#887) when pass indices of wrong type.

bpo-10030: Sped up reading encrypted ZIP files by 2 times. (#550)

bc79e9d

bpo-29946: Fix "sqrtpi defined but not used" (#908)

d99f0ee

bpo-29928: Add f-string to the Glossary (GH-864)

2341664

Remove catching OSError in ftphandler test. Only URLError is raised i…

a5136fd

…n urllib.request module. (#918)

bpo-29952: Use usual terminology of dict (GH-917)

20b8322

s/keys and elements/keys and values/

suppress compiler warnings in _ctypes_test (#902)

1b04170

Changed test code to suppress a compiler warning, while taking care to avoid the code being optimized out by the compiler.

Fix spurious MemoryError introduced by PR #886. (#930)

f6418e8

Fix MemoryError caused by moving around code in PR #886; nbytes was sometimes used unitinitalized (in non-debug builds, when use_calloc was false and elsize was 0).

bpo-29953: Fix memory leaks in the replace() method of datetime and t…

b1446d1

…ime (#927) objects when pass out of bound fold argument.

Link to the devguide's PR guidelines (GH-932)

25a2dd7

bpo-29763: Use unittest cleanup in test_site (GH-841)

cf01ec0

Add helpful explaination to test_password_manager tests. (#936)

67d0a63

Also uncomment and fix a path test.

Revert "bpo-29763: Use unittest cleanup in test_site (GH-841)" (GH-942)

14aa5c3

This reverts commit b94d7fd.

bpo-29949: Fix set memory usage regression (GH-943)

918227e

Revert "Minor factoring: move redundant resize scaling logic into the resize function." This reverts commit 4897300.

bpo-29692: bpo-29692: contextlib.contextmanager may incorrectly uncha…

d5b4965

…in RuntimeError

soolabettu closed this Apr 1, 2017

izumiberat mentioned this pull request Jun 9, 2022

Preventing warning when installing dependencies on Mac OS X #93658

Closed

Uh oh!

bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeE… #891

bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeE… #891

Uh oh!

Conversation

soolabettu commented Mar 30, 2017

Uh oh!

mention-bot commented Mar 30, 2017

Uh oh!

soolabettu commented Mar 30, 2017

Uh oh!

JelleZijlstra commented Mar 30, 2017

Uh oh!

soolabettu commented Mar 30, 2017

Uh oh!

soolabettu commented Mar 30, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soolabettu commented Mar 30, 2017

Uh oh!

brettcannon commented Mar 31, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soolabettu commented Mar 31, 2017

Uh oh!

soolabettu commented Apr 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

brettcannon Mar 30, 2017 •

edited

Loading