Skip to content

bpo-44116: Add GC support to _csv heap types - #26074

Merged
pablogsal merged 4 commits into
python:mainfrom
erlend-aasland:gc-csv
May 12, 2021
Merged

bpo-44116: Add GC support to _csv heap types#26074
pablogsal merged 4 commits into
python:mainfrom
erlend-aasland:gc-csv

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented May 12, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor
  • Implement full GC protocol support for _csv heap types
  • Use tp_clear iso. tp_finalize to clear type contexts

Petr's reproducer now outputs the following:

$ ./python.exe bug.py
73166
73168
73168
73168
73168
73168
73166
73168
73168
73168

However, the _csv test suite now leaks (EDIT: updated output after b8772d6)
UPDATE: No leaks after 19e2952

https://bugs.python.org/issue44116

@erlend-aasland

erlend-aasland commented May 12, 2021

Copy link
Copy Markdown
ContributorAuthor

(I'm applying skip news to this, @pablogsal (@corona10), as it is similar to GH-26072.)

Fidget-Spinner
Fidget-Spinner reviewed May 12, 2021
View reviewed changes

@Fidget-SpinnerFidget-Spinner 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.

Thanks! Some minor nits below:

Comment threadModules/_csv.c Outdated
Comment threadModules/_csv.c Outdated
@erlend-aasland
erlend-aasland marked this pull request as ready for review May 12, 2021 16:55
@Fidget-Spinner

Fidget-Spinner commented May 12, 2021

Copy link
Copy Markdown
Member

Doh I should've refreshed before I submitted the review. Didn't see your latest commits :(. Sorry.

@erlend-aasland

erlend-aasland commented May 12, 2021

Copy link
Copy Markdown
ContributorAuthor

Doh I should've refreshed before I submitted the review. Didn't see your latest commits :(. Sorry.

No problem, Ken :) I believe all your comments were addressed in 19e2952.

@erlend-aaslanderlend-aasland changed the title [WIP] bpo-44116: Add GC support to _csv heap typesbpo-44116: Add GC support to _csv heap typesMay 12, 2021
pablogsal
pablogsal reviewed May 12, 2021
View reviewed changes
Comment threadModules/_csv.c Outdated
pablogsal
pablogsal reviewed May 12, 2021
View reviewed changes
Comment threadModules/_csv.c Outdated
@pablogsal

pablogsal commented May 12, 2021

Copy link
Copy Markdown
Member

One thing that I don't understand is why the CSV tests when executed under the refleak checker don't show any problems

@pablogsalpablogsal added the needs backport to 3.10 only security fixes label May 12, 2021
@pablogsal

pablogsal commented May 12, 2021

Copy link
Copy Markdown
Member

Does this need backport to 3.9?

@pablogsal
pablogsal merged commit e5ba1fe into python:mainMay 12, 2021
@miss-islington

miss-islington commented May 12, 2021

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot

bedevere-bot commented May 12, 2021

Copy link
Copy Markdown

GH-26081 is a backport of this pull request to the 3.10 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.10 only security fixes label May 12, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 12, 2021
Erlend Egeberg Aasland@miss-islington
bpo-44116: Add GC support to _csv heap types (pythonGH-26074)
911bfef
(cherry picked from commit e5ba1fe)
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@erlend-aasland
erlend-aasland deleted the gc-csv branch May 12, 2021 18:21
@erlend-aasland

erlend-aasland commented May 12, 2021

Copy link
Copy Markdown
ContributorAuthor

Does this need backport to 3.9?

No. This was added in December in GH-23224

@erlend-aasland

erlend-aasland commented May 12, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Thanks for reviewing, Pablo and Ken!

pablogsal pushed a commit that referenced this pull request May 12, 2021
@miss-islington
bpo-44116: Add GC support to _csv heap types (GH-26074) (GH-26081)
ba260ac
(cherry picked from commit e5ba1fe)
@encukou

encukou commented May 12, 2021

Copy link
Copy Markdown
Member

Thank you for pushing this through!

@erlend-aaslanderlend-aasland mentioned this pull request May 13, 2021
Merged
@pablogsalpablogsal mentioned this pull request Jul 18, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsalpablogsalpablogsal approved these changes
@Fidget-SpinnerFidget-SpinnerFidget-Spinner approved these changes
@encukouencukouAwaiting requested review from encukou

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@erlend-aasland@Fidget-Spinner@pablogsal@miss-islington@bedevere-bot@encukou@the-knights-who-say-ni