bpo-12029: Exception handling should match subclasses #6461
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
206c1b9 to
9f931da
Compare
|
|
602b2ce to
76e17c9
Compare
|
@the-knights-who-say-ni can I get a re-run of the CLA script? I'm verified now. |
76e17c9 to
24153ce
Compare
|
Ok, I've run a bench mark to test the performance hit. For the benchmark code below that tests the "long path" in exception handling, I'm seeing a 1.4% slowdown over 100 million runs. To be precise, I'm seeing
Here's a graph that batches the runs into 100000 trial each and shows the variability in runs:
The slowdown, while real, is not particularly significant: There are many cases where the code in You can reproduce this analysis via the code: import time
import json
time_us = 0
num_runs = 1000
num_tests_per_run = 100000
stats = []
for n in range(num_runs):
start_time = time.time()
for i in range(num_tests_per_run):
try:
{}["a"]
except TypeError:
pass
except ValueError:
pass
except KeyError:
pass
end_time = time.time()
time_us = (end_time - start_time) / num_tests_per_run * 1e6
stats.append(time_us)
time.sleep(1e-4) # Make less sensitive to initial OS state
print("Took {} µs per test".format(sum(stats)/len(stats)))
import os
filename = os.path.abspath("stats-{}.json".format(time.time()))
print("Writing to file {}".format(filename))
with open(filename, 'w') as fp:
json.dump(stats, fp) |
|
2 years on, still no fix 😢 |
|
Rejected in favour of a doc note, see ticket and #32027 |
Exception handling should match subclasses, not subtypes
Example
Background and evidence of bug-ness
Issue 2534 [1] (10 years ago!) introduced the behavior, but only in the Python 3 patch [2]. During code review, the correct function call was used [3], but the function's name got switched in the final python3 patch without any comment.
The current Python 2 code uses
PyObject_IsSubclass, and produces the correct behavior in the example above (using__metaclass__ = ABCMeta, of course). This leads me to strongly suspect that this is a bug, not a feature. The note below regarding unittest for further evidence that this code has eight legs.Given the ancient nature of this bug, it affects all versions of python3.
[1] https://bugs.python.org/issue2534
[2] https://bugs.python.org/file11257/isinstance3k-2.patch
[3] https://codereview.appspot.com/483/diff/1/21#newcode114
[4] https://github.com/python/cpython/blob/2.7/Python/errors.c#L119
Solution
Coming very soon in a PR on Github, but in short, we do the following:
PyType_IsSubtypetoPyObject_IsSubclass.unittest’sself.assertRaisesfunction usesissubclassand does not alert to this bug. (Different symptom, same cause.)-Mike
https://bugs.python.org/issue33271
https://bugs.python.org/issue12029