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.

Sign up for GitHub

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

Jump to bottom

bpo-32710: Fix _overlapped.Overlapped memory leaks #11489

Merged
merged 2 commits into from
Jan 11, 2019
Merged

bpo-32710: Fix _overlapped.Overlapped memory leaks #11489

merged 2 commits into from
Jan 11, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 10, 2019 •
edited
Loading

Fix memory leaks in asyncio ProactorEventLoop on overlapped operation
failure.

Changes:

  • Implement the tp_clear and tp_traverse slots in the
    _overlapped.Overlapped type to help to break reference cycles and
    identify referrers in the garbage collector.
  • Always clear overlapped on failure: set type to TYPE_NOT_STARTED
    and release resources.

https://bugs.python.org/issue32710

@vstinner
Copy link
Member Author

I don't think that this change deserves a NEWS entry.

serhiy-storchaka
serhiy-storchaka reviewed Jan 10, 2019
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I suspect that there are leaks when self->type is set to TYPE_NOT_STARTED.

@asvetlov
Copy link
Contributor

allocated_buffer and user_buffer are members of mutually exclusive C union type.
self->type is used to specify what union member is valid now.

@vstinner
Copy link
Member Author

I suspect that there are leaks when self->type is set to TYPE_NOT_STARTED.

Oh right... My previous memory leak fix was incomplete :-( I updated this PR to always clear the overlapped on failure, but also implemen tp_clear slot.

Does it look better now?

@vstinner vstinner changed the title bpo-32710: Add tp_traverse to _overlapped.Overlapped bpo-32710: Fix _overlapped.Overlapped memory leaks Jan 10, 2019
@vstinner
bpo-32710: Fix _overlapped.Overlapped memory leaks
34c1c1f 
Fix memory leaks in asyncio ProactorEventLoop on overlapped operation
failure.

Changes:

* Implement the tp_traverse slot in the _overlapped.Overlapped type
  to help to break reference cycles and identify referrers in the
  garbage collector.
* Always clear overlapped on failure: not only set type to
  TYPE_NOT_STARTED, but release also resources.
@vstinner
Copy link
Member Author

I changed my mind. It's wrong to implement tp_clear: if a buffer memory is release while an overlapped is still running, the process will crash. In practice, we shouldn't get into a case where tp_clear is called on a running ("pending") overlapped operation, but I prefer to be pendantic because overlapped are very tricky and error prone.

I amended my change to be able to edit the commit message: and to no longer define tp_clear.

@serhiy-storchaka
Copy link
Member

It is OK to not implement tp_clear. Not every object that provides tp_traverse should provide tp_clear.

Maybe make allocated_buffer and user_buffer not members of a union? Then you can traverse/clear without depending on type.

@vstinner
Copy link
Member Author

Maybe make allocated_buffer and user_buffer not members of a union? Then you can traverse/clear without depending on type.

I don't think that it would simplify much the code. An overlapped is used for a single action and then is destroyed. Its type shouldn't change, and I'm fine with the current switch() to factorize the code.

@vstinner vstinner merged commit 5485085 into python:master Jan 11, 2019
@vstinner vstinner deleted the overlapped_traverse branch January 11, 2019 13:35
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-11519 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 11, 2019
@vstinner @miss-islington
bpo-32710: Fix _overlapped.Overlapped memory leaks (pythonGH-11489)
756baee 
Fix memory leaks in asyncio ProactorEventLoop on overlapped operation
failures.

Changes:

* Implement the tp_traverse slot in the _overlapped.Overlapped type
  to help to break reference cycles and identify referrers in the
  garbage collector.
* Always clear overlapped on failure: not only set type to
  TYPE_NOT_STARTED, but release also resources.
(cherry picked from commit 5485085)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
miss-islington added a commit that referenced this pull request Jan 11, 2019
@miss-islington @vstinner
bpo-32710: Fix _overlapped.Overlapped memory leaks (GH-11489)
059997d 
Fix memory leaks in asyncio ProactorEventLoop on overlapped operation
failures.

Changes:

* Implement the tp_traverse slot in the _overlapped.Overlapped type
  to help to break reference cycles and identify referrers in the
  garbage collector.
* Always clear overlapped on failure: not only set type to
  TYPE_NOT_STARTED, but release also resources.
(cherry picked from commit 5485085)

Co-authored-by: Victor Stinner <vstinner@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvetlov asvetlov asvetlov approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@1st1 1st1 Awaiting requested review from 1st1

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vstinner @asvetlov @serhiy-storchaka @miss-islington @bedevere-bot @the-knights-who-say-ni