-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix stack overflow by resetting recursion limit after IPython import. #9805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Can one of the admins verify this patch? Admins can comment |
Hrm, this is interesting. It feels wrong to me for a library to change a value like this. I'm surprised to learn that Jedi does it. cc'ing IPython dev @Carreau to see if he has recommendations/thoughts. |
Yes, I was also surprised, you can see the relevant code here: https://github.com/davidhalter/jedi/blob/3602c10916a29d19e7b5b89cfc6d8970b8bb488e/jedi/api/__init__.py#L48 It seems like it would make more sense to increase the limit only around specific code that requires it (I guess the _analysis function is probably what this is for) and reset it afterwards. Probably most code doesn't depend on the recursion limit being handled properly, and I think this may not be a problem on other OSes due to larger default stacks, so I expect this hasn't been a high-priority issue for anyone. |
If we're going to do this then I'd suggest two things:
|
I think if there's a "right" place to fix this, it's probably in Jedi, since this is essentially Jedi leaking global state to everything that imports it. I don't know much about Jedi, though, so someone who's familiar with it would need to figure out how to make this more targeted. |
(Author of Jedi here) I probably agree that Jedi should deal with this. I will have to investigate how expensive setting the recursion limit is. This should ideally not be done at all, but for Jedi this is kind of needed unfortunately. We could however try setting this only upon entering type inference or something similar. It's however not that easy to do that, because Jedi has tons of entry points. I never really thought that this could be an issue, because Jedi in its original form was thought to do static analysis and would therefore most likely not run arbitrary software next to it in the same process. Once we started doing the Python Shell/IPython integration, this was obviously not true anymore. So, don't expect results soon, but this is definitely something that we should change. |
I wrote a bit more in davidhalter/jedi#1902 (comment) While I think this is definitely an issue on Jedi's side, I want to understand one thing: You just want to have a |
I can try to add a workaround in IPython as well and add a context manager to set the recursion limit when we enter/exit jedi, my main concern is I believe the type inference of completion is a generator, so we may need to enter/exit for each item which may have a cost, if like @davidhalter said setting the recursion limit has a cost. locally sysetrecursionlimit/getrecustionlimit take ~15ns. It does not seem to be smart and skip setting it if it's already the same value, though is fairly strait forward so I'm not sure it can grow to much slower. |
That's pretty fast, it makes definitely sense to set it in Jedi in that case. |
Thank you @Carreau and @davidhalter for engaging here. Feel free to keep engaging on this PR for conversation, but I'm probably going to close this for now. This is, I think, clearly an upstream issue and it looks like upstream is handling it. Thank you all. |
pre-commit run --all-files