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

Projects
None yet
4 participants
@paulnivin
Contributor

paulnivin commented Mar 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Mar 18, 2016

Collaborator

💯

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

Collaborator

tilgovi commented Mar 18, 2016

💯

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

@benoitc

This comment has been minimized.

Show comment
Hide comment
Owner

benoitc commented Mar 18, 2016

+1

@paulnivin

This comment has been minimized.

Show comment
Hide comment
@paulnivin

paulnivin Mar 18, 2016

Contributor

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

Contributor

paulnivin commented Mar 18, 2016

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

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Mar 18, 2016

Collaborator

@paulnivin did you push?

Collaborator

tilgovi commented Mar 18, 2016

@paulnivin did you push?

@paulnivin

This comment has been minimized.

Show comment
Hide comment
@paulnivin

paulnivin Mar 18, 2016

Contributor

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.

Contributor

paulnivin commented Mar 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@paulnivin

paulnivin Mar 19, 2016

Contributor

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. 😑

Contributor

paulnivin commented Mar 19, 2016

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

This comment has been minimized.

@berkerpeksag

berkerpeksag Mar 19, 2016

Collaborator

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!

@berkerpeksag

berkerpeksag Mar 19, 2016

Collaborator

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!

This comment has been minimized.

@paulnivin

paulnivin Mar 20, 2016

Contributor

Added a comment and squashed the commits.

@paulnivin

paulnivin Mar 20, 2016

Contributor

Added a comment and squashed the commits.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Mar 19, 2016

Collaborator

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. 😜

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. 😜

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Mar 20, 2016

Collaborator

Excellent. Thanks, again.

Collaborator

tilgovi commented Mar 20, 2016

Excellent. Thanks, again.

tilgovi added a commit that referenced this pull request Mar 20, 2016

Merge pull request #1228 from lyft/circular-ref-async-fd-leak-fix
Fix delayed socket close caused by circular ref from assigning traceback to local var

@tilgovi tilgovi merged commit 5b32dde into benoitc:master Mar 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

Merge pull request #1228 from lyft/circular-ref-async-fd-leak-fix
Fix delayed socket close caused by circular ref from assigning traceback to local var
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment