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

Break circular reference to exc_info when exception is thrown #1936

Merged
merged 4 commits into from
Mar 26, 2017

Conversation

homm
Copy link
Contributor

@homm homm commented Jan 27, 2017

This fixes #1872

I also updated tested script, it is much easier to find circular references:

https://gist.github.com/homm/c20a7dc74f78d393f6ffac537532fd93

Without fix:

$ ab -c 100 -n 10000 http://localhost:8888/dummy/
Requests per second:    1265.13 [#/sec] (mean)
Percentage of the requests served within a certain time (ms)
  50%     80
  66%     81
  75%     82
  80%     83
  90%     88
  95%    102
  98%    106
  99%    111
 100%    197 (longest request)

$ time curl http://localhost:8888/collect/
Collected: 690000
real	0m0.477s

With fix:

$ ab -c 100 -n 10000 http://localhost:8888/dummy/
Requests per second:    1346.77 [#/sec] (mean)
Percentage of the requests served within a certain time (ms)
  50%     76
  66%     77
  75%     78
  80%     79
  90%     82
  95%     88
  98%     91
  99%     92
 100%     94 (longest request)

$ time curl http://localhost:8888/collect/
Collected: 0
real	0m0.020s

@homm
Copy link
Contributor Author

homm commented Jan 27, 2017

There are a lot more circular references in python 3. I'm not sure why and how to fix it simpler.

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

Thanks, this is awesome. I'm going to take your testing script and check it in to the repo as well so I'll be able to find it in the future.

@@ -193,7 +193,11 @@ def exec_in(code, glob, loc=None):
if PY3:
exec("""
def raise_exc_info(exc_info):
raise exc_info[1].with_traceback(exc_info[2])
try:
Copy link
Member

Choose a reason for hiding this comment

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

Why did this only change in the python 3 version? Doesn't the py2 version have the same circular reference?

Copy link

Choose a reason for hiding this comment

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

@bdarnell Any answer to your question? I'm getting memory leaks (python 2.7)

Copy link
Member

Choose a reason for hiding this comment

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

I put my best guess in #1936 (comment). Please do not post comments on long-closed issues. Normally I'd say you should open a new issue with details about your situation, but since tornado's support for python 2.7 ended in 2019, I'm afraid you're on your own now.

Copy link

@ndvbd ndvbd Dec 3, 2022

Choose a reason for hiding this comment

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

I tried adding the finally statement - didn't work. I tried upgrading python 2.7.12->2.7.17 - didn't work. Are the memory leak issues solved in python3 for Tornado?
Anyhow I see a leak with 2.7.18+5.1.1, when changing tornado 5.1.1->4.5.3 - problem is solved.

@bdarnell bdarnell merged commit b642b83 into tornadoweb:master Mar 26, 2017
bdarnell added a commit that referenced this pull request Mar 26, 2017
@bdarnell
Copy link
Member

BTW, my guess about why this is more complicated on python 3 is the fact that raising an exception in an except or finally block in python 3 preserves the previous exception's traceback too. We might be able to solve it by adding from None to certain raise statements going back and trying to do that in a different way (I tried reverting your py3 commit and adding from None to raise_exc_info, but that wasn't enough).

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.

Possible memory leak
3 participants