Skip to content
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

Fix delayed socket close caused by circular ref from assigning traceback to local var #1228

Merged
merged 1 commit into from Mar 20, 2016

Conversation

paulnivin
Copy link
Contributor

As part of pulling in 2e231d2 and a3f6df5, EPIPEs result in the EnvironmentError exception handler in handle() being run. In many environments, EPIPEs can be extremely common. The exception handler stores the traceback into a local variable, creating a circular reference, and prevents the system socket (holding a fd open along with other socket resources) from being closed until the next Python full garbage collection cycle. The behavior of sys.exec_info() is documented at https://docs.python.org/2/library/sys.html#sys.exc_info with a large red warning statement. This new exception path for EPIPEs can create significant additional resource usage for gunicorn installations.

Removing the assignment into exc_info causes the socket fd to be freed quickly (handled by ref counting), as was the case before the two prior patches were merged.

This commit also fixes the ssl.SSLError path.

@tilgovi
Copy link
Collaborator

tilgovi commented Mar 18, 2016

💯

@paulnivin there are a few other places where this happens. Want to 🔥 them all?

@benoitc
Copy link
Owner

benoitc commented Mar 18, 2016

+1

@paulnivin
Copy link
Contributor Author

I'll fix up the other places as well and update the branch.

@tilgovi
Copy link
Collaborator

tilgovi commented Mar 18, 2016

@paulnivin did you push?

@paulnivin
Copy link
Contributor Author

Not yet. Will shortly. I'm going through the codebase and fixing up all the uses of sys.exec_info(). The async EnvironmentError path was the only one that caused a dramatic change in gunicorn performance/resource utilization for us.

@paulnivin
Copy link
Contributor Author

Updated the branch to clean up all the calls to sys.exc_info() to conform to recommended best practices. We're not running 9392e7a in production, but it passes the gunicorn tests. 😑

tb_string = traceback.format_exc(exc_tb)
self.wsgi = util.make_fail_app(tb_string)
finally:
del exc_tb
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a comment to explain what del exc_tb does here (and perhaps a reference to this PR).

Could you also squash the commits into one?

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment and squashed the commits.

@tilgovi
Copy link
Collaborator

tilgovi commented Mar 19, 2016

Also, sorry @paulnivin, I misread your comment and thought it said you already had fixed up the other places when I asked if you had pushed. Wasn't trying to be pushy. 😜

@paulnivin paulnivin force-pushed the circular-ref-async-fd-leak-fix branch from 9392e7a to 20d3a2d Compare March 20, 2016 00:33
@tilgovi
Copy link
Collaborator

tilgovi commented Mar 20, 2016

Excellent. Thanks, again.

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.

None yet

4 participants