GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-120754: Fix memory leak in FileIO.__init__() by vstinner · Pull Request #124225 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Sep 18, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Free 'self->stat_atopen' before assigning it, since io.FileIO.init() can be called multiple times manually (especially by test_io).

  • Issue: Speed up open().read() pattern by reducing the number of system calls #120754

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
pythongh-120754: Fix memory leak in FileIO.__init__()
bcb790f
Free 'self->stat_atopen' before assigning it, since
io.FileIO.__init__() can be called multiple times manually
(especially by test_io).
@vstinner vstinner added the skip news label Sep 18, 2024
@bedevere-app bedevere-app Bot added the awaiting core review label Sep 18, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Sep 18, 2024
Speed up open().read() pattern by reducing the number of system calls #120754
Closed
@vstinner vstinner mentioned this pull request Sep 18, 2024
gh-120754: Refactor I/O modules to stash whole stat result rather than individual members #123412
Merged
@vstinner

vstinner commented Sep 18, 2024

Copy link
Copy Markdown
Member Author

cc @cmaloney

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cmaloney

cmaloney commented Sep 18, 2024

Copy link
Copy Markdown
Contributor

👍 Definitely where the leak is happening / my own tracking finds same location. https://vstinner.github.io/debug-python-refleak.html really helpful in tracking down, thanks for writing up.

There's a number of cases where self->fd is changed but the cached information about the file (now stat_atopen, previously estimated_size and _blksize) aren't changed. I started tracking those down (https://github.com/python/cpython/compare/main...cmaloney:cpython:cmaloney/fix_closefd_norealloc?expand=1). Happy with a much more minimal fix to get bots green. Trying to find a refactor so all the closefd logic which feels fairly replicated across the code at the moment can hopefully get deduped a lot while fixing potential corner case issues.

vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cmaloney
cmaloney approved these changes Sep 18, 2024
View reviewed changes
@vstinner
vstinner merged commit 43cd7aa into python:main Sep 18, 2024
@vstinner
vstinner deleted the fileio_init branch September 18, 2024 22:11
@bedevere-app bedevere-app Bot removed the awaiting core review label Sep 18, 2024
@vstinner

vstinner commented Sep 18, 2024

Copy link
Copy Markdown
Member Author

(https://github.com/python/cpython/compare/main...cmaloney:cpython:cmaloney/fix_closefd_norealloc?expand=1)

Only allocating (PyMem_New) stat_atopen if it's NULL sounds like a better fix than my fix. I merged my fix anyway since you approved it and I would like to repair buildbot as soon as possible.

You might include your better fix in a following PR if you want, since you planned more changes for io if I understood correctly.

Thanks for reviewing my fix!

cmaloney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cmaloney

cmaloney commented Sep 18, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Yep, will make more PRs stat_atopen got several patches moving again.

  1. isatty at open to replace GH-120754: Remove isatty call during regular open #121593 and fix Avoid calling isatty() for most open() calls #90102
  2. "clear out cached stat result anytime self->fd is changed" (followup from this PR)
  3. Potentially clear out stat_atopen on seek Unbounded reads by zipfile may cause a MemoryError. #113977. That is a potentially more comprehensive solution to code that does file.seek() then file.read(), but also in theory the "check position in file if file is large" code should make unnecessary, https://github.com/python/cpython/blob/main/Modules/_io/fileio.c#L769-L783. Still trying to wrap my head around why the "get position" didn't seem to work for that issue)
vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
@vstinner @savannahostrowski
pythongh-120754: Fix memory leak in FileIO.__init__() (python#124225)
a0cf0ec
Free 'self->stat_atopen' before assigning it, since
io.FileIO.__init__() can be called multiple times manually
(especially by test_io).
@cmaloney cmaloney mentioned this pull request Oct 8, 2024
gh-120754: Ensure _stat_atopen is cleared on fd change #125166
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmaloney cmaloney cmaloney approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@vstinner @cmaloney
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.