FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-32710: Fix _overlapped.Overlapped memory leaks by vstinner · Pull Request #11489 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Merged
vstinner merged 2 commits into
python:masterfrom
vstinner:overlapped_traverse
Jan 11, 2019
ConversationCommits (2)ChecksFiles changed
Merged

bpo-32710: Fix _overlapped.Overlapped memory leaks#11489
vstinner merged 2 commits into
python:masterfrom
vstinner:overlapped_traverse

Conversation

vstinner commented Jan 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 requested a review from asvetlov January 10, 2019 00:17

vstinner commented Jan 10, 2019

Copy link
Copy Markdown
Member Author

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

serhiy-storchaka reviewed Jan 10, 2019
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

asvetlov commented Jan 10, 2019

Copy link
Copy Markdown
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 commented Jan 10, 2019

Copy link
Copy Markdown
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 changed the title bpo-32710: Add tp_traverse to _overlapped.Overlapped bpo-32710: Fix _overlapped.Overlapped memory leaks Jan 10, 2019
serhiy-storchaka reviewed Jan 10, 2019
View reviewed changes
Comment thread Modules/overlapped.c Outdated
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 commented Jan 10, 2019

Copy link
Copy Markdown
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 commented Jan 10, 2019

Copy link
Copy Markdown
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 commented Jan 10, 2019

Copy link
Copy Markdown
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 merged commit 5485085 into python:master Jan 11, 2019
vstinner deleted the overlapped_traverse branch January 11, 2019 13:35

miss-islington commented Jan 11, 2019

Copy link
Copy Markdown
Contributor

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

bedevere-bot commented Jan 11, 2019

Copy link
Copy Markdown

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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

asvetlov asvetlov approved these changes
serhiy-storchaka serhiy-storchaka approved these changes
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

Back | FazBrowse Home | New Git URL