Skip to content

Handle RecursionError exception in perform_request (issue 1602) #1607

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

Closed
wants to merge 5 commits into from

Conversation

hmilkovi
Copy link
Contributor

@hmilkovi hmilkovi commented Jun 4, 2021

Propagate RecursionError if it happends on lower levels so when used like:

import sys
import inspect
import elasticsearch

es_client = elasticsearch.Elasticsearch("http://localhost:9200/")
print(f"Recursion limit: {sys.getrecursionlimit()}")

def rec():
    print(len(inspect.stack()))
    es_client.index(index='testing', body={'test': 'test'}, request_timeout=1)
    rec()

rec()

doesn't crash python interpreter with core dump.

Personal opinion is that this is really needed when we you web servers like uwsgi, gunicorn as
then the whole web server dies and doesn't return http 500 status code.

Related issue: #1602

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 4, 2021

❌ Author of the following commits did not sign a Contributor Agreement:
e182f58, , 508b935, 9a08a30,

Please, read and sign the above mentioned agreement if you want to contribute to this project

@sethmlarson
Copy link
Contributor

sethmlarson commented Jun 4, 2021

Gotcha, so the problem really is that we're swallowing the RecursionError out of except Exception:? If that's the case let's do something simpler, instead the very first branch under except Exception: check for RecursionError and reraise without modifying the exception.

Also you'll need to sign the CLA for me to merge.

@hmilkovi
Copy link
Contributor Author

hmilkovi commented Jun 4, 2021

Yes that's the case :) . Does accepting CLA take some time to propagate or as I have signed it?

@sethmlarson sethmlarson closed this Jun 4, 2021
@sethmlarson sethmlarson reopened this Jun 4, 2021
@sethmlarson
Copy link
Contributor

@hmilkovi Did you sign the CLA with the same email address as the one used in your Git commits?

@hmilkovi
Copy link
Contributor Author

hmilkovi commented Jun 5, 2021

CLA check (read up in docs that I need to comment again)

@hmilkovi
Copy link
Contributor Author

hmilkovi commented Jun 5, 2021

I have changed email on my last commit. Do I need to change it on all or is it easier to make this changes on new branch and new PR?
I want to contribute as private entity with private email.

Sorry for bothering you with that.

@sethmlarson
Copy link
Contributor

Yeah creating a new PR is probably best

@hmilkovi
Copy link
Contributor Author

hmilkovi commented Jun 6, 2021

Closing this one for #1609 .
Now CLA is fine :)

@hmilkovi hmilkovi closed this Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants