sys.setrecursionlimit() must fail if the current recursion depth is higher than the new low-water mark #69461
Comments
|
haypo@selma$ ./python -m test -R 3:3: test_sys Current thread 0x00007f1921854700 (most recent call first): Current thread 0x00007f1921854700 (most recent call first): |
|
Oh, it's even worse: "./python -m test test_sys" is enough to reproduce the crash. It looks like the crashs was introduced by the following change which (indirectly) adds one more Python frame in the code to execute test_sys. changeset: 98417:281ab7954d7c Add a new Regrtest.run_test() method to ensure that --coverage pass the same |
vstinner
changed the title
|
New changeset 00c1cd1f0131 by Victor Stinner in branch 'default': |
|
At revision 281ab7954d7c, the test_recursionlimit_recovery() test of test_sys is executed at depth 36. The overflowed flag is reset when the depth becomes smaller than 50 * 3 // 4 = 37, so when Py_LeaveRecursiveCall() is called with depth level 36. The test raises a RecursionError once which sets the overflowed flag. Then it tries again to raise the RecursionError, but instead Py_FatalError() is called because the overflowed flag was not reset. |
|
See also bpo-25222. _Py_CheckRecursiveCall may be being called recursively through Py_FatalError. |
serhiy-storchaka
added
interpreter-core
|
New changeset 60c4fd84ef92 by Victor Stinner in branch '3.4': New changeset 898a9a959927 by Victor Stinner in branch '3.5': New changeset bae0912dd160 by Victor Stinner in branch 'default': |
|
Attached patch raises a ValueError if the current recursion depth is higher than the new "low-water mark". The low-water mark is the value computed by _Py_MakeEndRecCheck() in Py_LeaveRecursiveCall() to decide if we can reset the overflowed field of the thread state to 0. Example of error with the patch: $ ./python -c 'import sys; sys.setrecursionlimit(2)'
Traceback (most recent call last):
File "<string>", line 1, in <module>
ValueError: cannot set recursion limit to 2: the minimum recursion limit is 1 at the recursion depth 1 |
vstinner
changed the title
This message looks confusing to me. 2 > 1, isn't? The dependency of min_limit from new_limit is not monotonic: new_limit = 99 => min_limit = 74
new_limit = 100 => min_limit = 75
new_limit = 101 => min_limit = 51 |
|
sys_setrecursionlimit-2.patch: fix error message, exchange the last 2 PyErr_Format() parameters. |
|
Serhiy wrote:
Ok, now I'm confused too :-) First of all, I propose to change the exception type to RecursionError because it's really strange to get a ValueError depending on the current recursion depth. I would prefer to have an hardcoded minimum limit instead of a minimum depending on the current recursion depth, but I'm not sure that it's technically possible according to current constraints in CPython. Updated patch (version 3) doesn't mention the computed "minimum limit" in the error message since it's hard to compute it for the user (and even for me :-)). For the user, it's hard to estimate (or compute) the current recursion depth. Updated example: $ ./python -c 'import sys; sys.setrecursionlimit(0)'
Traceback (most recent call last):
File "<string>", line 1, in <module>
ValueError: recursion limit must be greater or equal than 1
$ ./python -c 'import sys; sys.setrecursionlimit(1)'
Traceback (most recent call last):
File "<string>", line 1, in <module>
RecursionError: cannot set recursion limit to 1 at the recursion depth 1: the limit is too low
$ ./python -c 'import sys; sys.setrecursionlimit(2)'
Traceback (most recent call last):
File "<string>", line 1, in <module>
RecursionError: cannot set recursion limit to 2 at the recursion depth 1: the limit is too low$ ./python -c 'import sys; sys.setrecursionlimit(3)'(A limit of 3 is valid.) |
Right, _Py_MakeEndRecCheck() is not monotonic. Let me try to make it monotonic: def f1(x): return x * 3 // 4
def f2(x): return x - 50f1() > f2() for x <= 196 So I propose to switch between f1() and f2() at x>200 for _Py_MakeEndRecCheck(). It gives: recursion depth => low-water mark 25 => 18 Attached end_rec_check.patch makes _Py_MakeEndRecCheck() monotonic. |
|
_Py_MakeEndRecCheck() was changed in bpo-5392 (noised Antoine as a committer). There are many ways to make it monotonic. #define _Py_MakeEndRecCheck(x) \
(--(x) < ((_Py_CheckRecursionLimit > 200) \
? (_Py_CheckRecursionLimit - 25) \
: (3 * (_Py_CheckRecursionLimit >> 2))))
#define _Py_MakeEndRecCheck(x) \
(--(x) < ((_Py_CheckRecursionLimit > 100) \
? (_Py_CheckRecursionLimit - 50) \
: (_Py_CheckRecursionLimit >> 1)))Since the formula is so complex, it would be worth to extract common part from _Py_MakeEndRecCheck() and sys_setrecursionlimit(). Otherwise the code can become desynchronized. The error message still looks confusing to me. May be short it to "limit is too low"? Noised David as grammar expert. Why not have an hardcoded minimum limit? Current recursion depth and therefore minimal recursion limit are implementation defined and can depend on a way how the module is executed (run a script, import a module, run with runpy or IDLE, etc). May be interpret the limit relatively to current recursion depth? |
|
sys_setrecursionlimit-4.patch:
|
Oh, I didn't know this issue. If I am right, it's a duplicate of this issue, even if it was a little bit different because the lower-water mark was computed differently.
Since I don't understand well this issue, I prefer to keep the formula unchanged for low limit (smaller than 100, or smaller than 200 with my patch) to keep the 3/4 ratio.
Right, it's now done in my new patch.
I prefer to really explain the fact that the RecursionError is raised depending on the current recursion depth. Otherwise, I'm quite sure that someone will complain that "too low" have different values on the same PC but on two different applications.
I'm not sure that you understood correctly the issue. The root cause is that you cannot call sys.setrecursionlimit(X) at recursion depth Y. There is no constant "minimum limit": it depends on the recusion depth because of how Python handles recursion error (the overflowed flag). An alternative would be to remove completly the overflowed flag with its fatal error. It was introduced during large refactoring of Python, maybe the bug cannot occur anymore? Again, since I don't know the whole history of how Python handles recursion error, I prefer the conservative approach of changing the minimum amount of code.
Hum, I dislike this idea. I prefer to keep an absolute value. Otherwise, it would make Python more surprising. |
By the way, it doesn't exist in Python 2 at all. Try attached double_recursion_error.py program. $ python2 double_recursion_error.py
first recursion error
second recursion error
$ python3 double_recursion_error.py
first recursion error
Fatal Python error: Cannot recover from stack overflow.Current thread 0x00007f80a6985700 (most recent call first): |
|
The overflowed flag was introduced 8 years ago (near the release of Python 3.0) by the following changeset: changeset: 42013:cd125fe83051 In testing, a number of crashes occurred as code would There are still some places where both str and str8 are |
|
New changeset eb0c76442cee by Victor Stinner in branch '3.5': |
|
I pushed sys_setrecursionlimit-4.patch. Thanks a lot Serhiy for your review and your help on this issue! |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: