Skip to content

Fix memory leak in module greendns (alternative to #682)#811

Merged
itamarst merged 7 commits intoeventlet:masterfrom
jiajunsu:fix_issue_810
Dec 20, 2023
Merged

Fix memory leak in module greendns (alternative to #682)#811
itamarst merged 7 commits intoeventlet:masterfrom
jiajunsu:fix_issue_810

Conversation

@jiajunsu
Copy link
Copy Markdown
Contributor

@jiajunsu jiajunsu commented Sep 20, 2023

Change EAI_*_ERROR to lambda expression, and initialize them
when being called, to avoid memory leak with __traceback__.tb_next

fixes #810

temoto
temoto previously requested changes Oct 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@temoto temoto left a comment

Choose a reason for hiding this comment

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

@jiajunsu thank you very much for attention to this problem.

I believe if we're going to break compat with external code by changing public variable type, it's better to do it loud. Would not want somebody to have isinstance(err, eventlet.support.greendns.EAI_EAGAIN_ERROR) always False because it's a function now.

Please do either:

  • _new_eagain_error = lambda ...
  • or EAI_EAGAIN_MESSAGE = "Lookup timed out"; raise socket.gaierror(socket.EAI_AGAIN, EAI_EAGAIN_MESSAGE)

@jiajunsu
Copy link
Copy Markdown
Contributor Author

jiajunsu commented Oct 28, 2023 •
edited
Loading

@jiajunsu thank you very much for attention to this problem.

I believe if we're going to break compat with external code by changing public variable type, it's better to do it loud. Would not want somebody to have isinstance(err, eventlet.support.greendns.EAI_EAGAIN_ERROR) always False because it's a function now.

Please do either:

  • _new_eagain_error = lambda ...
  • or EAI_EAGAIN_MESSAGE = "Lookup timed out"; raise socket.gaierror(socket.EAI_AGAIN, EAI_EAGAIN_MESSAGE)

Thanks for the comment. I just tried another way to keep EAI_*_ERROR to be instances, adding a function raise_new_error for raising error.

Besides, if we call isinstance(err, EAI_EAGAIN_ERROR), it will raise an exception right now, because EAI_EAGAIN_ERROR is an instance rather than type.
TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union

@jiajunsu jiajunsu requested a review from temoto November 1, 2023 01:18
@jiajunsu
Copy link
Copy Markdown
Contributor Author

jiajunsu commented Nov 2, 2023

@temoto I've passed the testcases of tests/greendns_test.py with py{37|38|39|311}, please have a review, thank you.

@damani42
Copy link
Copy Markdown

damani42 commented Dec 12, 2023

Looks good to merge.

4383
4383 reviewed Dec 18, 2023
View reviewed changes
4383
4383 reviewed Dec 18, 2023
View reviewed changes
@itamarst itamarst mentioned this pull request Dec 19, 2023
Closed
@itamarst itamarst changed the title Fix memory leak in module greendns Fix memory leak in module greendns (alternative to #682) Dec 19, 2023
@jiajunsu jiajunsu requested a review from 4383 December 20, 2023 02:03
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 20, 2023 •
edited
Loading

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (53502e5) 53% compared to head (c98a6af) 53%.

Files Patch % Lines
eventlet/support/greendns.py 50% 7 Missing ⚠️
Additional details and impacted files 
@@          Coverage Diff          @@
##           master   #811   +/-   ##
=====================================
- Coverage      53%    53%   -1%     
=====================================
  Files          88     88           
  Lines        9881   9883    +2     
  Branches     1852   1852           
=====================================
+ Hits         5335   5336    +1     
- Misses       4156   4157    +1     
  Partials      390    390
Flag Coverage Δ
ipv6 22% <7%> (-1%) ⬇️
py310epolls 52% <50%> (+<1%) ⬆️
py310poll 52% <50%> (+<1%) ⬆️
py310selects 52% <50%> (+<1%) ⬆️
py311epolls 52% <50%> (+<1%) ⬆️
py312epolls 50% <50%> (+<1%) ⬆️
py37epolls 50% <50%> (+<1%) ⬆️
py38epolls 52% <50%> (+<1%) ⬆️
py38openssl 51% <50%> (-1%) ⬇️
py38poll 52% <50%> (+<1%) ⬆️
py38selects 52% <50%> (+<1%) ⬆️
py39dnspython1 50% <50%> (+<1%) ⬆️
py39epolls 52% <50%> (+<1%) ⬆️
py39poll 52% <50%> (+<1%) ⬆️
py39selects 52% <50%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

4383
4383 approved these changes Dec 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@4383 4383 left a comment

Choose a reason for hiding this comment

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

LGTM

@4383
Copy link
Copy Markdown
Member

4383 commented Dec 20, 2023

Codecov is flaky...

@4383
Copy link
Copy Markdown
Member

4383 commented Dec 20, 2023

@itamarst any preference between this PR and #682?

@itamarst
Copy link
Copy Markdown
Contributor

itamarst commented Dec 20, 2023

This one has a test, so may as well be this one.

@itamarst itamarst removed the request for review from temoto December 20, 2023 16:55
@itamarst itamarst merged commit c5f4684 into eventlet:master Dec 20, 2023
@jiajunsu jiajunsu deleted the fix_issue_810 branch December 21, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@4383 4383 4383 approved these changes

@itamarst itamarst itamarst approved these changes

@temoto temoto temoto left review comments

Assignees

No one assigned

Labels

bug important

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory leak in greendns

5 participants

@jiajunsu @damani42 @4383 @itamarst @temoto